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 (file)
index 0000000..4e0eb45
--- /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 3a102745010fb23a5a4dcd38ac26c74c99d6ea39..d2bc8f703faba90fd4c2e177605bbb0d2d8e1c4d 100644 (file)
--- 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 1fbaf59654cc9cc1c06aeb33ef6808014fb422ea..87f549f514bc92c4edfee64fd935a09971eb92f7 100644 (file)
--- 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 e254752091ec3deebbe9dab3d2fcc40be7cc4af4..4dec1988cc9b0dd51a9b9e3e32561b1524d268e0 100644 (file)
--- 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):