twisted.enterprise.adbapi.ConnectionPool does not raise ConnectionLost when cp_reconnect is set

[bernie9998]

When running a database action with ConnectionPool.runInteraction or runOperation or runQuery and the cp_reconnect flag is set, ConnectionPool fails to raise the ConnectionLost exception as expected.

Instead, the ConnectionLost exception is discarded and the original exception thrown by the dbapi driver is raised instead.

This is not ideal as it requires a different check for each database implementation to determine if a request needs to be retried after a disconnect. If the ConnectionLost exception was raised as expected, a simpler check would be possible which would be independent of database implementation.

[bernie9998]

Patch for bug

[bernie9998]

I have added a patch which would solve this issue-- in _runInteration and _runWithConnection, added a check for the except block to determine if the exception is a connectionlost, and then raise that exception instead of the original if that is the case.

[exarkun]

Hi bernie, thanks for the patch. I wonder if you could also include some unit tests for this change?

[bernie9998]

patch with new test cases for ConnectionLost when reconnect is set

[bernie9998]

I added another patch for twisted/test/test_adbapi.py.

It adds two new test cases to ConnectionPoolTestCase class:

test_runWithConnectionRaiseConnectionLostError and test_runInteractionRaiseConnectionLostError

Each test simulates a connection that raises a ConnectionLost on rollback and asserts that that failure instead of the original is passed up by the connection pool.

[exarkun]

Thanks!

[itamar]

I'm not sure I accept the premise for this patch. If you've got an exception back, that means it was thrown by the function you passed to runInteraction. In that case adbapi will *always* rollback. So either rollback succeeded and then connection was lost, or connection was lost before rollback. In the latter case the database will have rollbacked the transaction automatically.

In other words, if you got an exception back, you know that the transaction wasn't executed (at least in situations this patch is dealing with). Switching to ConnectionLost would hide underlying exception, thereby losing information: it was either deliberately raised by the developer or was caused by a bug.

Summary: ConnectionLost adds no additional information, since you always have to retry if you got exception, and actually hides useful information.

[exarkun]

Some of that should maybe go into the docs?

[bernie9998]

Actually, a ConnectionLost exception has nothing to do with the running of the function in runInteraction and is nothing more than a transient exception caused by the database dropping the connection due to inactivity. While the cp_reconnect flag causes the pool to automatically attempt a reconnect for the connection, it does not attempt to resend the original database query. As a result, there needs to be a mechanism to allow the application to catch this case and enable it to resend the query itself.

I disagree with your broad statement that one should always retry after an exception. I'd say most exceptions are the result of a malformed statement or an unsatisfied constraint. In such cases, retrying would only result in the same error. With such cases, the only proper action would be to log the exception and consider the it a bad request. A connection lost is the only case in which the request should be retried.

The original exception is only discarded if the resulting rollback fails, as that is where the ConnectionLost exception is raised. I can understand concern that the ConnectionLost would be raised in cases where the connection was not actually lost. I'm assuming the exception is automatically raised on a rollback failure because the original developer that wrote the code assumed that the only error case for a failed rollback was from a connection being lost.

[itamar]

Certainly we need better docs, but actually there is a general code issue here too. When you run a transaction and it fails you want two pieces of information: the interaction function's exception (if any), and the rollback()'s exception (if any). Rollback exception implies connection was lost. Right now we can only return one of those.

Bernie: If the connection was lost before interaction started, rather than in between interaction and rollback, both exceptions will be present. But both exceptions will also be present if user had a bad database query and then connection was lost in between that and doing rollback (unlikely but possible - or maybe query was so bad the database closed the connection in self defense, e.g. custom protocol implementation may be buggy). So in these rare situations user would lose important information if we just raised ConnectionLost.

What's more, the ConnectionLost hides the *reason* rollback failed, which may also be interesting (in particular, timeout vs. connection closed can be informative. ask me how much I hate Oracle and stupid firewall configs that drop connections without sending RST).

It may be time to bite the bullet and do a rewrite of ConnectionPool to provide a cleaner API.

[bernie9998]

While a rewrite of the ConnectionPool would be a proper long term solution-- we can have it automatically handle inactivity disconnects and not require the application to re-attempt.

That said, a short term fix to this issue could be to attach the original exception as an attribute to the connection lost exception. While not ideal, it would at least enable the application to have access to the underlying exception.

That said, I still think that the ConnectionLost exception is still quite useful in the majority of cases. In the event of a disconnect due to inactivity, the application should at the very least have a way of detecting and handling it without having to check for different exceptions from all possible database implementations.

Perhaps a rewritten ConnectionPool should include a set of standard exceptions for most common error cases (ConnectionLost, ForeignKeyConstraintError, etc) and translate all connector exceptions to these error cases. The application should not have to handle anything on the connector level other than specifying to the ConnectionPool what connector to use.

[itamar]

Adding masked exception as attribute of ConnectionLost would help not lose information, yes.

The problem that still remains at that point is that you're suggesting an API improvement that is arguably backwards incompatible. I feel bad making it hard for you to improve things, but I also don't want to break existing applications. Especially since most people I suspect write single-database apps, not generic multi-database ones.

I would argue a rewritten ConnectionPool should not attempt to have a set of standard exceptions, because adbapi doesn't attempt to mask database differences; the goal is just event loop integration. I would suggest runInteraction Deferred could return either a successful result, or a single possible exception class: TransactionFailed, with attributes for underlying exception (if any), rollback exception (if any), and possibly indicating whether the connection was closed/reopened.

In other words, what you proposed a layer to abstract away database differences, my suggestion is limited to exposing whatever internal activity the ConnectionPool did on top of adbapi.

(You could of course build another layer on top of that that knew details about specific dbapi libraries and how to abstract away those errors, but that's not really something we'd want as part of our base library. For that matter, it shouldn't necessarily be part of Twisted (or Twisted-dependent) at all, since non-Twisted DBAPI users could in theory benefit from underlying database abstraction. The worst difference is the stupid support for three styles of SQL quoting... it's possible someone's already written an abstraction layer of that sort though.)

[bernie9998]

Yes, standardizing db exceptions probably should be a python improvement and would probably fair better as a plep proposal.

And I completely agree that we should not break existing behavior. In twisted versions before 10, the behavior of the connection pool was to throw the ConnectionLost exception as documented. That behavior broke with version 10.

[itamar]

Ugh. *All* the choices suck :/

I'm tempted to just suggest adding two new methods with a decent API, say "transact()" and "execute()", as a first step towards a more thorough rewrite.

[Screwtape]

Let me see if I can summarize the state of this ticket so far:

• In Twisted releases prior to 10.0, if ConnectionPool's cp_reconnect flag was set, then ConnectionPool methods could sometimes raise the Twisted-specific ConnectionLost connection if, after an exception, a given connection couldn't be rolled back to a sensible state.
• In Twisted 10.0, this behaviour was changed so that if an exception occurred while executing a query, and rollback failed, the original exception would be returned instead of ConnectionLost.
• If we stick with the new behaviour, we have a regression (which presumably is how bernie9998 noticed this change in the first place).
• If we revert to the old behaviour, application authors that send bogus or problematic queries to their database won't be able to get at the exceptions their queries throw, which makes things... difficult to debug.

Frankly, I'm not sure why Twisted has an ConnectionLost exception at all - it seems to me that if the DBAPI module raises an exception and a rollback isn't possible, the ConnectionPool should just quietly cross that connection off its list and create another - and if it runs out of connections and can't create any more, trying to interact with the database should raise whatever exception the DBAPI module raises on a failed connection attempt. Of course, that would be a pretty big backwards-incompatible change too.

It sounds like the short-term way forward here is to write a patch that looks like the current patch, but adds the original exception as an attribute of the ConnectionLost exception.

[itamar]

I more or less agree with the summary, except I'd be more likely to characterize pre-10.0 behavior as a bug rather than a regression. OTOH it's a bug that was around so long that perhaps it counts as part of the API :/