From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Thu, 8 Feb 2018 19:52:29 +0000 (-0500)
Subject: Ensure weakref finalize_fairy operates upon the current connection
X-Git-Tag: rel_1_1_16~6^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6c6f865541a7b3bd984cf0fb5cb5f511f7971758;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Ensure weakref finalize_fairy operates upon the current connection

Fixed a fairly serious connection pool bug where a connection that is
acquired after being refreshed as a result of a user-defined
:class:`.DisconnectionError` or due to the 1.2-released "pre_ping" feature
would not be correctly reset if the connection were returned to the pool by
weakref cleanup (e.g. the front-facing object is garbage collected); the
weakref would still refer to the previously invalidated DBAPI connection
which would have the reset operation erroneously called upon it instead.
This would lead to stack traces in the logs and a connection being checked
into the pool without being reset, which can cause locking issues.

Change-Id: Iabd9f3a63a1d0207d0de0054a6ced3560818cf9c
Fixes: #4184
(cherry picked from commit ab1f524c355c0bbac68485a60cb99e7a9d0f944a)
---

diff --git a/doc/build/changelog/unreleased_11/4184.rst b/doc/build/changelog/unreleased_11/4184.rst
new file mode 100644
index 0000000000..4e0eb453f2
--- /dev/null
+++ b/doc/build/changelog/unreleased_11/4184.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, pool
+    :tickets: 4184
+
+    Fixed a fairly serious connection pool bug where a connection that is
+    acquired after being refreshed as a result of a user-defined
+    :class:`.DisconnectionError` or due to the 1.2-released "pre_ping" feature
+    would not be correctly reset if the connection were returned to the pool by
+    weakref cleanup (e.g. the front-facing object is garbage collected); the
+    weakref would still refer to the previously invalidated DBAPI connection
+    which would have the reset operation erroneously called upon it instead.
+    This would lead to stack traces in the logs and a connection being checked
+    into the pool without being reset, which can cause locking issues.
+
diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py
index 3a10274501..d2bc8f703f 100644
--- a/lib/sqlalchemy/pool.py
+++ b/lib/sqlalchemy/pool.py
@@ -525,7 +525,7 @@ class _ConnectionRecord(object):
             fairy,
             lambda ref: _finalize_fairy and
             _finalize_fairy(
-                dbapi_connection,
+                None,
                 rec, pool, ref, echo)
         )
         _refs.add(rec)
@@ -671,9 +671,11 @@ def _finalize_fairy(connection, connection_record,
     """
     _refs.discard(connection_record)
 
-    if ref is not None and \
-            connection_record.fairy_ref is not ref:
-        return
+    if ref is not None:
+        if connection_record.fairy_ref is not ref:
+            return
+        assert connection is None
+        connection = connection_record.connection
 
     if connection is not None:
         if connection_record and echo:
diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py
index 1fbaf59654..87f549f514 100644
--- a/test/engine/test_pool.py
+++ b/test/engine/test_pool.py
@@ -1749,6 +1749,40 @@ class QueuePoolTest(PoolTestBase):
 
         self._assert_cleanup_on_pooled_reconnect(dbapi, p)
 
+    @testing.requires.predictable_gc
+    def test_userspace_disconnectionerror_weakref_finalizer(self):
+        dbapi, pool = self._queuepool_dbapi_fixture(
+            pool_size=1,
+            max_overflow=2)
+
+        @event.listens_for(pool, "checkout")
+        def handle_checkout_event(dbapi_con, con_record, con_proxy):
+            if getattr(dbapi_con, 'boom') == 'yes':
+                raise tsa.exc.DisconnectionError()
+
+        conn = pool.connect()
+        old_dbapi_conn = conn.connection
+        conn.close()
+
+        eq_(old_dbapi_conn.mock_calls, [call.rollback()])
+
+        old_dbapi_conn.boom = 'yes'
+
+        conn = pool.connect()
+        dbapi_conn = conn.connection
+        del conn
+        gc_collect()
+
+        # new connection was reset on return appropriately
+        eq_(dbapi_conn.mock_calls, [call.rollback()])
+
+        # old connection was just closed - did not get an
+        # erroneous reset on return
+        eq_(
+            old_dbapi_conn.mock_calls,
+            [call.rollback(), call.close()]
+        )
+
     @testing.requires.timing_intensive
     def test_recycle_pool_no_race(self):
         def slow_close():
diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py
index e254752091..4dec1988cc 100644
--- a/test/engine/test_reconnect.py
+++ b/test/engine/test_reconnect.py
@@ -12,6 +12,7 @@ from sqlalchemy.testing import fixtures
 from sqlalchemy.testing.engines import testing_engine
 from sqlalchemy.testing.mock import Mock, call, patch
 from sqlalchemy import event
+from sqlalchemy.testing.util import gc_collect
 
 
 class MockError(Exception):