clean out XID state on connection.close()

[Mike Bayer]

On Friday, 10 January 2014 08:06:16 UTC-8, Michael Bayer wrote: maybe. I was thinking, is the Session really doing the right thing here by not getting involved, but I think yeah that still might be appropriate. so we’d need to carry along some extra information about the transaction with the connection so that do_rollback() and do_commit() can pick up on that.

or maybe session.close() needs to do some bookkeeping with the SessionTransaction. not sure.

The call stack here is:

-> session.close() sqlalchemy/orm/session.py(999)close() -> transaction.close() sqlalchemy/orm/session.py(437)close() -> connection.close() sqlalchemy/engine/base.py(583)close() -> conn.close() sqlalchemy/pool.py(579)close() -> self.checkin() sqlalchemy/pool.py(506)checkin() -> self._pool, None, self._echo, fairy=self) sqlalchemy/pool.py(432)_finalize_fairy() -> pool._dialect.do_rollback(fairy)

The tpc state (xid, is_prepared) is held on the TwoPhaseTransaction object, referenced (self.__transaction) by the engine Connection object which calls conn.close(). I don't think SessionTransaction need be involved here, but perhaps the engine Connection / Transaction should. Some options are:

1. In engine Connection.close(), when self.__transaction.is_active pass down xid, is_prepared to the _ConnectionFairy. That would imply muddying the DBAPI Connection contract of the engine Connection.__connection.

2. Make the engine Transaction aware of the configured reset_on_return behaviour so that Transaction.close() can either rollback or commit. Connection.close() could then call Transaction.close().

3. Keep track of xid, is_prepared on the pool's connection_record.

[Mike Bayer]

9c64607572a04eb2ed7981db8999

[Mike Bayer]

more adjustments and fixes in bebf30e34d669a5ede54e512e55.

[Mike Bayer]

• Fixed some "double invalidate" situations were detected where a connection invalidation could occur within an already critical section like a connection.close(); ultimately, these conditions are caused by the change in 2907, in that the "reset on return" feature calls out to the Connection/Transaction in order to handle it, where "disconnect detection" might be caught. However, it's possible that the more recent change in 2985 made it more likely for this to be seen as the "connection invalidate" operation is much quicker, as the issue is more reproducible on 0.9.4 than 0.9.3.

Checks are now added within any section that an invalidate might occur to halt further disallowed operations on the invalidated connection. This includes two fixes both at the engine level and at the pool level. While the issue was observed with highly concurrent gevent cases, it could in theory occur in any kind of scenario where a disconnect occurs within the connection close operation. fixes #3043 ref #2985 ref #2907

• add some defensive checks during an invalidate situation: 1. _ConnectionRecord.invalidate might be called twice within finalize_fairy if the _reset() raises an invalidate condition, invalidates, raises and then goes to invalidate the CR. so check for this. 2. similarly within Conneciton, anytime we do handle_dbapi_error(), we might become invalidated. so a following finally must check self.__invalid before dealing with the connection any futher.

→ <<cset 85d1899b76a3>>