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

3 files changed, 91 insertions, 18 deletions

diff --git a/doc/build/changelog/unreleased_13/5034.rst b/doc/build/changelog/unreleased_13/5034.rst
new file mode 100644
index 000000000..0f33b4883
--- /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 ac495f321..e331e2512 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.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 a2e88e156..3ad8880de 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()"""
@@ -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(),
             ],
         )