AbortOperation issue

[stoffman]

There is a mismatch between the return values from AbortOperation (I refer to the last version I'm using but that seems to be going on for some time https://github.com/frones/ZeosLib/tree/ ... ffe888/src)

TZSQLiteConnection.AbortOperation always returns 1

TZPostgreSQLConnection.AbortOperation set the default to 1 but on success returns 0

TZOracleConnection.AbortOperation only returns 0 (raises exception on fail)

TZFirebirdConnection.AbortOperation while I cannot test it, seems to return 1 on success and 0 and failure

What should be the correct behavior in that case, with regard to the other parts of zeos?

[marsupilami]

Hello stoffman,

even though I didn't look at the code, I would expect that the return value of AbortOperation is irrelevant and that an Exception is raised if AbortOperation doesn't work.

A note on the branch of Zeos that you refer to: Please use https://github.com/marsupilami79/zeoslib. It is more or less our official copy and is kept up to date automatically.

Best regards,

Jan

[stoffman]

It feels like this was copied form JDBC design where you have checked exceptions, so the user knows to try and catch AbortOperation. But in pascal it makes less sense

At least in that regard the sqlite driver behave differently has it doesn't raise any exception.

Also in TZAbstractConnection:

Code: Select all

function TZAbstractConnection.AbortOperation: Boolean;
begin
 Result := FConnection.AbortOperation = 0;
end; 

which means that it will always return false for sqlite.

[aehimself]

Was a long time ago but I think I copied the behavior from Ping... which SHOULD mean 0 on success, 1 on error. Later on we introduced EZNotSupportedException, we also could implement that; makes sense if the driver doesn't (yet) support aborting.

I'll take a closer look tomorrow.

[aehimself]

As it turns out yes, return values were not used correctly in every driver. I issued a pull request on GitHub which attempts to fix this.

However, as I analyzed the code Firebird is completely wrong. I put some notes there explaining why it needs to be reworked... Unfortunately I don't have Firebird available to be able to check and test it.

[marsupilami]

I applied the changes to Zeos trunk and Zeos 8.0-patches

[stoffman]

I would also like to recommend a change of code in TZDbcPooledConnection.AbortOperation , The way it works now is that if it doesn't have a connection it obtain one only to abort it immediately. This is a waste. It should first check whatever the connection IsClosed and if not try to abort it. if it is close it should return success.

[marsupilami]

However, as I analyzed the code Firebird is completely wrong. I put some notes there explaining why it needs to be reworked... Unfortunately I don't have Firebird available to be able to check and test it.

I use Firebird on a daily basis. What tests should I do? Should we try to integrate tests into the test suite?

[aehimself]

I use Firebird on a daily basis. What tests should I do?

Jan,

This is the comment I put in .AbortOperation for Firebird:

Code: Select all

    // This is a mess and probably doesn't work as intended. cIStatus_RESULT_OK is 0, which means it doesn't matter what
    // getState returns, anything AND 0 will be 0. Therefore, 0 <> 0 will always return FALSE and Ord(False) is always 0.
    // Result is always 0, so FStatus.init will never be called...
    Result := Ord((Fstatus.getState and cIStatus_RESULT_OK) <> 0);
    if Result <> 0 then
      FStatus.init;

Delphi 12's LSP went nuts at this point so I could not really identify what FStatus is and what methods it has.
What I'd do is to try to call .AbortOperation on Firebird and check what this GetStatus returns. Maybe we can start from there to try to untangle the correct way...

Should we try to integrate tests into the test suite?

Shortly after implementing .AbortOperation I remember writing some SLEEP queries for MSSQL, MySQL and maybe for Oracle so we can check if aborting is actually successful. Those could be turned into tests for the suite.

[aehimself]

I would also like to recommend a change of code in TZDbcPooledConnection.AbortOperation , The way it works now is that if it doesn't have a connection it obtain one only to abort it immediately. This is a waste. It should first check whatever the connection IsClosed and if not try to abort it. if it is close it should return success.

Good catch! Ripe pull request is ready for picking!