Guard against re-entrant autobegin in Core, ORM

Fixed bug in "future" version of :class:`.Engine` where emitting SQL during
the :meth:`.EngineEvents.do_begin` event hook would cause a re-entrant
condition due to autobegin, including the recipe documented for SQLite to
allow for savepoints and serializable isolation support.

Fixed issue in new :class:`_orm.Session` similar to that of the
:class:`_engine.Connection` where the new "autobegin" logic could be
tripped into a re-entrant state if SQL were executed within the
:meth:`.SessionEvents.after_transaction_create` event hook.

Also repair the new "testing_engine" pytest fixture to
set up for "future" engine appropriately, which wasn't working
leading to the test_execute.py tests not using the future
engine since recent f1e96cb0874927a475d0c11139.

Fixes: #5845
Change-Id: Ib2432d8c8bd753e24be60720ec47affb2df15a4a

diff --git a/doc/build/changelog/unreleased_14/5845.rst b/doc/build/changelog/unreleased_14/5845.rst
new file mode 100644 (file)
index 0000000..4d9ad0c
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5845.rst
@@ -0,0 +1,19 @@
+.. change::
+    :tags: bug, engine, sqlite
+    :tickets: 5845
+
+    Fixed bug in the 2.0 "future" version of :class:`.Engine` where emitting
+    SQL during the :meth:`.EngineEvents.begin` event hook would cause a
+    re-entrant (recursive) condition due to autobegin, affecting among other
+    things the recipe documented for SQLite to allow for savepoints and
+    serializable isolation support.
+
+
+.. change::
+    :tags: bug, orm, regression
+    :tickets: 5845
+
+    Fixed issue in new :class:`_orm.Session` similar to that of the
+    :class:`_engine.Connection` where the new "autobegin" logic could be
+    tripped into a re-entrant (recursive) state if SQL were executed within the
+    :meth:`.SessionEvents.after_transaction_create` event hook.
\ No newline at end of file

diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index 72d66b7c82567a5c83c5fe54463f185eaa02e3dd..31bf885dbbe0b12cae7d74ed58abb0772b159d74 100644 (file)
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -813,10 +813,11 @@ class Connection(Connectable):
         if self._echo:
             self.engine.logger.info("BEGIN (implicit)")
 
+        self.__in_begin = True
+
         if self._has_events or self.engine._has_events:
             self.dispatch.begin(self)
 
-        self.__in_begin = True
         try:
             self.engine.dialect.do_begin(self.connection)
         except BaseException as e:

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index e8312f393b072d409ecb6f63eb1cc1472d4d2ce1..8a7a64f78c65c565f2a2d073702e609840ce6f67 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -531,6 +531,10 @@ class SessionTransaction(object):
 
         self._take_snapshot(autobegin=autobegin)
 
+        # make sure transaction is assigned before we call the
+        # dispatch
+        self.session._transaction = self
+
         self.session.dispatch.after_transaction_create(self.session, self)
 
     @property
@@ -1242,7 +1246,8 @@ class Session(_SessionClassMethods):
 
     def _autobegin(self):
         if not self.autocommit and self._transaction is None:
-            self._transaction = SessionTransaction(self, autobegin=True)
+            trans = SessionTransaction(self, autobegin=True)
+            assert self._transaction is trans
             return True
 
         return False
@@ -1299,7 +1304,7 @@ class Session(_SessionClassMethods):
         if self._transaction is not None:
             if subtransactions or _subtrans or nested:
                 trans = self._transaction._begin(nested=nested)
-                self._transaction = trans
+                assert self._transaction is trans
                 if nested:
                     self._nested_transaction = trans
             else:
@@ -1307,7 +1312,8 @@ class Session(_SessionClassMethods):
                     "A transaction is already begun on this Session."
                 )
         else:
-            self._transaction = SessionTransaction(self, nested=nested)
+            trans = SessionTransaction(self, nested=nested)
+            assert self._transaction is trans
         return self._transaction  # needed for __enter__/__exit__ hook
 
     def begin_nested(self):

diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py
index dcdeee5c902340a7897f082f09cfa27f77ac3acd..9141d9b1136cbc0d4a690b50cc11e06e74ccb0da 100644 (file)
--- a/lib/sqlalchemy/testing/fixtures.py
+++ b/lib/sqlalchemy/testing/fixtures.py
@@ -93,7 +93,7 @@ class TestBase(object):
         from . import engines
 
         def gen_testing_engine(
-            url=None, options=None, future=False, asyncio=False
+            url=None, options=None, future=None, asyncio=False
         ):
             if options is None:
                 options = {}

diff --git a/test/dialect/test_sqlite.py b/test/dialect/test_sqlite.py
index 587148525721f627136e8d7a9ad6a3f8bccbcf90..0500a20bd63f857d759dc0c60b4a1609545e8a3b 100644 (file)
--- a/test/dialect/test_sqlite.py
+++ b/test/dialect/test_sqlite.py
@@ -2430,6 +2430,10 @@ class SavepointTest(fixtures.TablesTest):
         connection.close()
 
 
+class FutureSavepointTest(fixtures.FutureEngineMixin, SavepointTest):
+    pass
+
+
 class TypeReflectionTest(fixtures.TestBase):
 
     __only_on__ = "sqlite"

diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py
index b9eb4f815e569503beaf9076a2e5ad84a1902972..7b997a017c9ee2667268ad98ff6d313a17bef836 100644 (file)
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -1417,6 +1417,27 @@ class EngineEventsTest(fixtures.TestBase):
         eq_(canary.be2.call_count, 1)
         eq_(canary.be3.call_count, 2)
 
+    def test_emit_sql_in_autobegin(self, testing_engine):
+        e1 = testing_engine(config.db_url)
+
+        canary = Mock()
+
+        @event.listens_for(e1, "begin")
+        def begin(connection):
+            result = connection.execute(select(1)).scalar()
+            canary.got_result(result)
+
+        with e1.connect() as conn:
+            assert not conn._is_future
+
+            with conn.begin():
+                conn.execute(select(1)).scalar()
+                assert conn.in_transaction()
+
+            assert not conn.in_transaction()
+
+        eq_(canary.mock_calls, [call.got_result(1)])
+
     def test_per_connection_plus_engine(self, testing_engine):
         canary = Mock()
         e1 = testing_engine(config.db_url)
@@ -2309,7 +2330,34 @@ class EngineEventsTest(fixtures.TestBase):
 
 
 class FutureEngineEventsTest(fixtures.FutureEngineMixin, EngineEventsTest):
-    pass
+    def test_future_fixture(self, testing_engine):
+        e1 = testing_engine()
+
+        assert e1._is_future
+        with e1.connect() as conn:
+            assert conn._is_future
+
+    def test_emit_sql_in_autobegin(self, testing_engine):
+        e1 = testing_engine(config.db_url)
+
+        canary = Mock()
+
+        @event.listens_for(e1, "begin")
+        def begin(connection):
+            result = connection.execute(select(1)).scalar()
+            canary.got_result(result)
+
+        with e1.connect() as conn:
+            assert conn._is_future
+            conn.execute(select(1)).scalar()
+
+            assert conn.in_transaction()
+
+            conn.commit()
+
+            assert not conn.in_transaction()
+
+        eq_(canary.mock_calls, [call.got_result(1)])
 
 
 class HandleErrorTest(fixtures.TestBase):

diff --git a/test/orm/test_events.py b/test/orm/test_events.py
index e85c23d6f6d831962ac02e04724aee668c37fb99..d9caa221d61e008cbf41ad7dc73c8225d3bc15ca 100644 (file)
--- a/test/orm/test_events.py
+++ b/test/orm/test_events.py
@@ -2173,6 +2173,22 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest):
         sess.rollback()
         eq_(assertions, [True, True])
 
+    @testing.combinations((True,), (False,))
+    def test_autobegin_no_reentrant(self, future):
+        s1 = fixture_session(future=future)
+
+        canary = Mock()
+
+        @event.listens_for(s1, "after_transaction_create")
+        def after_transaction_create(session, transaction):
+            result = session.execute(select(1)).scalar()
+            canary.got_result(result)
+
+        with s1.begin():
+            pass
+
+        eq_(canary.mock_calls, [call.got_result(1)])
+
     def test_flush_noautocommit_hook(self):
         User, users = self.classes.User, self.tables.users