From: Mike Bayer Date: Wed, 13 May 2020 16:42:08 +0000 (-0400) Subject: Assert reset agent always set correctly and is active X-Git-Tag: rel_1_3_17~1^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=332d30d03c6cae3ac12fc0b6a5aa1b319a62fec2;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Assert reset agent always set correctly and is active Fixed fairly critical issue where the DBAPI connection could be returned to the connection pool while still in an un-rolled-back state. The reset agent responsible for rolling back the connection could be corrupted in the case that the transaction was "closed" without being rolled back or committed, which can occur in some scenarios when using ORM sessions and emitting .close() in a certain pattern involving savepoints. The fix ensures that the reset agent is always active. Note for 1.3 the cherry-pick modifies the approach from master as transaction handling has diverged. Fixes: #5326 Change-Id: If056870ea70a2d9a1749768988d5e023f3061b31 (cherry picked from commit 4d161ac9c28986e1e022dfb93785767f28d6bfc8) --- diff --git a/doc/build/changelog/unreleased_13/5326.rst b/doc/build/changelog/unreleased_13/5326.rst new file mode 100644 index 0000000000..baebc18900 --- /dev/null +++ b/doc/build/changelog/unreleased_13/5326.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, engine + :tickets: 5326 + + Fixed fairly critical issue where the DBAPI connection could be returned to + the connection pool while still in an un-rolled-back state. The reset agent + responsible for rolling back the connection could be corrupted in the case + that the transaction was "closed" without being rolled back or committed, + which can occur in some scenarios when using ORM sessions and emitting + .close() in a certain pattern involving savepoints. The fix ensures that + the reset agent is always active. + diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index 296926abcb..3b4b4c2459 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -795,6 +795,23 @@ class Connection(Connectable): self.engine.dialect.do_savepoint(self, name) return name + def _discard_transaction(self, trans): + if trans is self.__transaction: + if trans._parent is trans: + self.__transaction = None + if self._still_open_and_connection_is_valid: + assert self.__connection._reset_agent is None + else: + self.__transaction = trans._parent + + # not doing this assertion for now, however this is how + # it would look: + # if self._still_open_and_connection_is_valid: + # trans = self._transaction + # while not trans._is_root: + # trans = trans._parent + # assert self.__connection._reset_agent is trans + def _rollback_to_savepoint_impl(self, name, context): assert not self.__branch_from @@ -1730,19 +1747,18 @@ class Transaction(object): an enclosing transaction. """ - if not self._parent.is_active: - return - if self._parent is self: + + if self._parent.is_active and self._parent is self: self.rollback() + self.connection._discard_transaction(self) def rollback(self): """Roll back this :class:`.Transaction`. """ - if not self._parent.is_active: - return - self._do_rollback() - self.is_active = False + if self._parent.is_active: + self._do_rollback() + self.is_active = False def _do_rollback(self): self._parent.rollback() diff --git a/lib/sqlalchemy/pool/base.py b/lib/sqlalchemy/pool/base.py index 5c4d4e5198..4f0bcf4968 100644 --- a/lib/sqlalchemy/pool/base.py +++ b/lib/sqlalchemy/pool/base.py @@ -880,7 +880,15 @@ class _ConnectionFairy(object): ", via agent" if self._reset_agent else "", ) if self._reset_agent: - self._reset_agent.rollback() + if not self._reset_agent.is_active: + util.warn( + "Reset agent is not active. " + "This should not occur unless there was already " + "a connectivity error in progress." + ) + pool._dialect.do_rollback(self) + else: + self._reset_agent.rollback() else: pool._dialect.do_rollback(self) elif pool._reset_on_return is reset_commit: @@ -891,7 +899,15 @@ class _ConnectionFairy(object): ", via agent" if self._reset_agent else "", ) if self._reset_agent: - self._reset_agent.commit() + if not self._reset_agent.is_active: + util.warn( + "Reset agent is not active. " + "This should not occur unless there was already " + "a connectivity error in progress." + ) + pool._dialect.do_commit(self) + else: + self._reset_agent.commit() else: pool._dialect.do_commit(self) diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py index 8b884d1101..d6baf19fb5 100644 --- a/test/engine/test_pool.py +++ b/test/engine/test_pool.py @@ -1691,6 +1691,8 @@ class ResetOnReturnTest(PoolTestBase): def __init__(self, conn): self.conn = conn + is_active = True + def rollback(self): self.conn.special_rollback() @@ -1722,6 +1724,8 @@ class ResetOnReturnTest(PoolTestBase): def __init__(self, conn): self.conn = conn + is_active = True + def rollback(self): self.conn.special_rollback() diff --git a/test/engine/test_transaction.py b/test/engine/test_transaction.py index 53a785e21c..6bab13109b 100644 --- a/test/engine/test_transaction.py +++ b/test/engine/test_transaction.py @@ -592,6 +592,63 @@ class ResetAgentTest(fixtures.TestBase): trans.commit() assert connection.connection._reset_agent is None + def test_trans_close(self): + with testing.db.connect() as connection: + trans = connection.begin() + assert connection.connection._reset_agent is trans + trans.close() + assert connection.connection._reset_agent is None + + def test_trans_reset_agent_broken_ensure(self): + eng = testing_engine() + conn = eng.connect() + trans = conn.begin() + assert conn.connection._reset_agent is trans + trans.is_active = False + + with expect_warnings("Reset agent is not active"): + conn.close() + + def test_trans_commit_reset_agent_broken_ensure(self): + eng = testing_engine(options={"pool_reset_on_return": "commit"}) + conn = eng.connect() + trans = conn.begin() + assert conn.connection._reset_agent is trans + trans.is_active = False + + with expect_warnings("Reset agent is not active"): + conn.close() + + @testing.requires.savepoints + def test_begin_nested_trans_close(self): + with testing.db.connect() as connection: + t1 = connection.begin() + assert connection.connection._reset_agent is t1 + t2 = connection.begin_nested() + assert connection.connection._reset_agent is t1 + assert connection._Connection__transaction is t2 + t2.close() + assert connection._Connection__transaction is t1 + assert connection.connection._reset_agent is t1 + t1.close() + assert connection.connection._reset_agent is None + assert not t1.is_active + + @testing.requires.savepoints + def test_begin_nested_trans_rollback(self): + with testing.db.connect() as connection: + t1 = connection.begin() + assert connection.connection._reset_agent is t1 + t2 = connection.begin_nested() + assert connection.connection._reset_agent is t1 + assert connection._Connection__transaction is t2 + t2.close() + assert connection._Connection__transaction is t1 + assert connection.connection._reset_agent is t1 + t1.rollback() + assert connection.connection._reset_agent is None + assert not t1.is_active + @testing.requires.savepoints def test_begin_nested_close(self): with testing.db.connect() as connection: diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py index 60ee7e8aac..33c511a37f 100644 --- a/test/orm/test_transaction.py +++ b/test/orm/test_transaction.py @@ -562,6 +562,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest): sess.rollback, ) + @testing.requires.independent_connections @testing.emits_warning(".*previous exception") def test_failed_rollback_deactivates_transaction(self): # test #4050 @@ -624,6 +625,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest): # outermost is active eq_(session.transaction._state, _session.ACTIVE) + @testing.requires.independent_connections @testing.emits_warning(".*previous exception") def test_failed_rollback_deactivates_transaction_ctx_integration(self): # test #4050 in the same context as that of oslo.db