Actively unset reset agent in discard transaction

zzzeek · zzzeek · commit 79de84b25e87 · 2020-05-14T10:51:29.000-04:00
The assumptions in _discard_transaction from 916e1fe were too narrow, assuming that if the given transaction were not our "current" one, that this would not be the reset agent. however as the legacy behvaior is that even a "nested" transaction gets set as "self._transaction", this did not accommodate for the nested transaction being thrown away. We will attempt to refine all of this logic in sqlalchemy#5327 for 1.4 /master assuming this is feasible for the full suite of current use cases. Fixes: sqlalchemy#5326 Change-Id: I6787e82c9e50c23317f87d0d094122c6a6f066da
diff --git a/doc/build/changelog/unreleased_13/5326.rst b/doc/build/changelog/unreleased_13/5326.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 5326
+
+    Further refinements to the fixes to the "reset" agent fixed in
+    :ticket:`5326`, which now emits a warning when it is not being correctly
+    invoked and corrects for the behavior.  Additional scenarios have been
+    identified and fixed where this warning was being emitted.
+
diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
@@ -821,25 +821,13 @@ def _discard_transaction(self, trans):
                 assert trans._parent is trans
                 self._transaction = None
 
-                # test suite w/ SingletonThreadPool will have cases
-                # where _reset_agent is on a different Connection
-                # entirely so we can't assert this here.
-                # if (
-                #    not self._is_future
-                #    and self._still_open_and_connection_is_valid
-                # ):
-                #    assert self.__connection._reset_agent is None
             else:
                 assert trans._parent is not trans
                 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
+        if not self._is_future and self._still_open_and_connection_is_valid:
+            if self.__connection._reset_agent is trans:
+                self.__connection._reset_agent = None
 
     def _rollback_to_savepoint_impl(
         self, name, context, deactivate_only=False
diff --git a/test/engine/test_transaction.py b/test/engine/test_transaction.py
@@ -663,7 +663,7 @@ def test_trans_commit_reset_agent_broken_ensure(self):
             conn.close()
 
     @testing.requires.savepoints
-    def test_begin_nested_trans_close(self):
+    def test_begin_nested_trans_close_one(self):
         with testing.db.connect() as connection:
             t1 = connection.begin()
             assert connection.connection._reset_agent is t1
@@ -677,6 +677,20 @@ def test_begin_nested_trans_close(self):
             assert connection.connection._reset_agent is None
         assert not t1.is_active
 
+    @testing.requires.savepoints
+    def test_begin_nested_trans_close_two(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._transaction is t2
+
+            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:
