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 (file)
index 0000000..baebc18
--- /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 296926abcbb5ac761cb625bda088412ddc73d0ce..3b4b4c2459cf364e185b51a9ba3d2a6219d77b85 100644 (file)
--- 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 5c4d4e51985d3caf515a2c758b97029b56d428c7..4f0bcf4968e1ee1d2b8d49344953bf7231b3c651 100644 (file)
--- 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 8b884d1101571e9a815fd3906eb39d84e1815cfe..d6baf19fb56a8e57b6e85e25fad990c66b314851 100644 (file)
--- 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 53a785e21c2d9a2daeb546ffcf4619a03009ff84..6bab13109b97634fae0333a850a1acf4695a915a 100644 (file)
--- 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 60ee7e8aac4bbb08564059213696822924f45f24..33c511a37f9180281472d96f5ed475a95a3682e7 100644 (file)
--- 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