From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 13 Dec 2019 17:44:23 +0000 (-0500)
Subject: Close connection if begin fails
X-Git-Tag: rel_1_3_12~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=beeb289696099d9284f61944de15c0763d861694;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

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
(cherry picked from commit ff471152b62f71accda62d9ede87e0107b8a8bcb)
---

diff --git a/doc/build/changelog/unreleased_13/5034.rst b/doc/build/changelog/unreleased_13/5034.rst
new file mode 100644
index 0000000000..0f33b48833
--- /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 971bc27862..9b3df21d59 100644
--- 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._contextual_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 913c223213..60ee7e8aac 100644
--- 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()"""