Close connection if begin fails

Fixed issue where by if the "begin" of a transaction failed at the Core
engine/connection level, such as due to network error or database is locked
for some transactional recipes, within the context of the :class:`.Session`
procuring that connection from the connection pool and then immediately
returning it, the ORM :class:`.Session` would not close the connection
despite this connection not being stored within the state of that
:class:`.Session`.  This would lead to the connection being cleaned out by
the connection pool weakref handler within garbage collection which is an
unpreferred codepath that in some special configurations can emit errors in
standard error.

Fixes: #5034
Change-Id: I6502a55791d86845f34bc10889c218f00765dfdc

diff --git a/doc/build/changelog/unreleased_13/5034.rst b/doc/build/changelog/unreleased_13/5034.rst
new file mode 100644 (file)
index 0000000..0f33b48
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/5034.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 5034
+
+    Fixed issue where by if the "begin" of a transaction failed at the Core
+    engine/connection level, such as due to network error or database is locked
+    for some transactional recipes, within the context of the :class:`.Session`
+    procuring that connection from the conneciton pool and then immediately
+    returning it, the ORM :class:`.Session` would not close the connection
+    despite this connection not being stored within the state of that
+    :class:`.Session`.  This would lead to the connection being cleaned out by
+    the connection pool weakref handler within garbage collection which is an
+    unpreferred codepath that in some special configurations can emit errors in
+    standard error.

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index ac495f321863392f82f250236ef5debc45ed54b9..e331e2512572b33d6c079870bc89dc12d0cf6755 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -415,6 +415,7 @@ class SessionTransaction(object):
                 )
             return self._connections[bind][0]
 
+        local_connect = False
         if self._parent:
             conn = self._parent._connection_for_bind(bind, execution_options)
             if not self.nested:
@@ -429,24 +430,32 @@ class SessionTransaction(object):
                     )
             else:
                 conn = bind.connect()
+                local_connect = True
 
-        if execution_options:
-            conn = conn.execution_options(**execution_options)
+        try:
+            if execution_options:
+                conn = conn.execution_options(**execution_options)
 
-        if self.session.twophase and self._parent is None:
-            transaction = conn.begin_twophase()
-        elif self.nested:
-            transaction = conn.begin_nested()
+            if self.session.twophase and self._parent is None:
+                transaction = conn.begin_twophase()
+            elif self.nested:
+                transaction = conn.begin_nested()
+            else:
+                transaction = conn.begin()
+        except:
+            # connection will not not be associated with this Session;
+            # close it immediately so that it isn't closed under GC
+            if local_connect:
+                conn.close()
+            raise
         else:
-            transaction = conn.begin()
-
-        self._connections[conn] = self._connections[conn.engine] = (
-            conn,
-            transaction,
-            conn is not bind,
-        )
-        self.session.dispatch.after_begin(self.session, self, conn)
-        return conn
+            self._connections[conn] = self._connections[conn.engine] = (
+                conn,
+                transaction,
+                conn is not bind,
+            )
+            self.session.dispatch.after_begin(self.session, self, conn)
+            return conn
 
     def prepare(self):
         if self._parent is not None or not self.session.twophase:

diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py
index a2e88e1562cfba92786dc18b2e19717ba91c9c93..3ad8880de2dd7be95ad9056457c387b07ffa18ba 100644 (file)
--- a/test/orm/test_transaction.py
+++ b/test/orm/test_transaction.py
@@ -405,6 +405,58 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
 
         eq_(len(sess.query(User).all()), 1)
 
+    def test_begin_fails_connection_is_closed(self):
+        eng = engines.testing_engine()
+
+        state = []
+
+        @event.listens_for(eng, "begin")
+        def do_begin(conn):
+            state.append((conn, conn.connection))
+            raise Exception("failure")
+
+        s1 = Session(eng)
+
+        assert_raises_message(Exception, "failure", s1.execute, "select 1")
+
+        conn, fairy = state[0]
+        assert not fairy.is_valid
+        assert conn.closed
+        assert not conn.invalidated
+
+        s1.close()
+
+        # close does not occur because references were not saved, however
+        # the underlying DBAPI connection was closed
+        assert not fairy.is_valid
+        assert conn.closed
+        assert not conn.invalidated
+
+    def test_begin_savepoint_fails_connection_is_not_closed(self):
+        eng = engines.testing_engine()
+
+        state = []
+
+        @event.listens_for(eng, "savepoint")
+        def do_begin(conn, name):
+            state.append((conn, conn.connection))
+            raise Exception("failure")
+
+        s1 = Session(eng)
+
+        s1.begin_nested()
+        assert_raises_message(Exception, "failure", s1.execute, "select 1")
+
+        conn, fairy = state[0]
+        assert fairy.is_valid
+        assert not conn.closed
+        assert not conn.invalidated
+
+        s1.close()
+
+        assert conn.closed
+        assert not fairy.is_valid
+
     def test_continue_flushing_on_commit(self):
         """test that post-flush actions get flushed also if
         we're in commit()"""
@@ -714,9 +766,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
             bind.mock_calls,
             [
                 mock.call.connect(),
-                mock.call.connect().execution_options(
-                    isolation_level="FOO"
-                ),
+                mock.call.connect().execution_options(isolation_level="FOO"),
                 mock.call.connect().execution_options().begin(),
             ],
         )