From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 12 Jun 2020 17:09:15 +0000 (-0400)
Subject: Warn when transaction context manager ends on inactive transaction
X-Git-Tag: rel_1_4_0b1~269
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5624430eb1d07c68d0931bc89f7146bc003fde19;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Warn when transaction context manager ends on inactive transaction

if .rollback() or .commit() is called inside the transaction
context manager, the transaction object is deactivated.
the context manager continues but will not be able to correctly
fulfill it's closing state.  Ensure a warning is emitted when
this happens.

Change-Id: I8fc3a73f7c21575dda5bcbd6fb74ddb679771630
---

diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index 3e02a29fec..81c0c9f580 100644
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -2095,6 +2095,9 @@ class RootTransaction(Transaction):
             ):
                 self.connection._dbapi_connection._reset_agent = None
 
+        elif self.connection._transaction is not self:
+            util.warn("transaction already deassociated from connection")
+
         # we have tests that want to make sure the pool handles this
         # correctly.  TODO: how to disable internal assertions cleanly?
         # else:
@@ -2133,7 +2136,7 @@ class RootTransaction(Transaction):
     def _connection_commit_impl(self):
         self.connection._commit_impl()
 
-    def _close_impl(self):
+    def _close_impl(self, try_deactivate=False):
         try:
             if self.is_active:
                 self._connection_rollback_impl()
@@ -2141,7 +2144,7 @@ class RootTransaction(Transaction):
             if self.connection._nested_transaction:
                 self.connection._nested_transaction._cancel()
         finally:
-            if self.is_active:
+            if self.is_active or try_deactivate:
                 self._deactivate_from_connection()
             if self.connection._transaction is self:
                 self.connection._transaction = None
@@ -2153,7 +2156,7 @@ class RootTransaction(Transaction):
         self._close_impl()
 
     def _do_rollback(self):
-        self._close_impl()
+        self._close_impl(try_deactivate=True)
 
     def _do_commit(self):
         if self.is_active:
diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py
index 041daf35e1..2cc34448d5 100644
--- a/lib/sqlalchemy/testing/fixtures.py
+++ b/lib/sqlalchemy/testing/fixtures.py
@@ -66,7 +66,8 @@ class TestBase(object):
         try:
             yield conn
         finally:
-            trans.rollback()
+            if trans.is_active:
+                trans.rollback()
             conn.close()
 
     # propose a replacement for @testing.provide_metadata.
diff --git a/test/engine/test_transaction.py b/test/engine/test_transaction.py
index 164604cd65..b82d143f91 100644
--- a/test/engine/test_transaction.py
+++ b/test/engine/test_transaction.py
@@ -320,6 +320,22 @@ class TransactionTest(fixtures.TestBase):
             1,
         )
 
+    def test_deactivated_warning_ctxmanager(self, local_connection):
+        with expect_warnings(
+            "transaction already deassociated from connection"
+        ):
+            with local_connection.begin() as trans:
+                trans.rollback()
+
+    @testing.requires.savepoints
+    def test_deactivated_savepoint_warning_ctxmanager(self, local_connection):
+        with expect_warnings(
+            "nested transaction already deassociated from connection"
+        ):
+            with local_connection.begin():
+                with local_connection.begin_nested() as savepoint:
+                    savepoint.rollback()
+
     def test_commit_fails_flat(self, local_connection):
         connection = local_connection
 
@@ -355,7 +371,10 @@ class TransactionTest(fixtures.TestBase):
 
         t1 = transaction[0]
         assert not t1.is_active
-        t1.rollback()  # no error
+        with expect_warnings(
+            "transaction already deassociated from connection"
+        ):
+            t1.rollback()  # no error
 
     @testing.requires.savepoints_w_release
     def test_savepoint_rollback_fails_flat(self, local_connection):