- Fixed critical issue whereby the pool "checkout" event handler

may be called against a stale connection without the "connect"
event handler having been called, in the case where the pool
attempted to reconnect after being invalidated and failed; the stale
connection would remain present and would be used on a subsequent
attempt.  This issue has a greater impact in the 1.0 series subsequent
to 1.0.2, as it also delivers a blanked-out ``.info`` dictionary to
the event handler; prior to 1.0.2 the ``.info`` dictionary is still
the previous one.
fixes #3497

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index bf04aabca672817390a27c078b7de00bd2b2bec6..2b8c5f3ad354fa612a0fb9b282765f599fe59199 100644 (file)
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -11,6 +11,24 @@
     .. include:: changelog_07.rst
         :start-line: 5
 
+.. changelog::
+    :version: 0.9.11
+
+    .. change::
+        :tags: bug, pool
+        :tickets: 3497
+        :versions: 1.0.8
+
+        Fixed critical issue whereby the pool "checkout" event handler
+        may be called against a stale connection without the "connect"
+        event handler having been called, in the case where the pool
+        attempted to reconnect after being invalidated and failed; the stale
+        connection would remain present and would be used on a subsequent
+        attempt.  This issue has a greater impact in the 1.0 series subsequent
+        to 1.0.2, as it also delivers a blanked-out ``.info`` dictionary to
+        the event handler; prior to 1.0.2 the ``.info`` dictionary is still
+        the previous one.
+
 .. changelog::
     :version: 0.9.10
     :released: July 22, 2015

diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py
index b38aefb3dae42ea44225cabc3ae1e53592f1eb1d..4dd954fc4954b0cd9c06a76cf764c0d7285d5d84 100644 (file)
--- a/lib/sqlalchemy/pool.py
+++ b/lib/sqlalchemy/pool.py
@@ -587,7 +587,12 @@ class _ConnectionRecord(object):
         if recycle:
             self.__close()
             self.info.clear()
+
+            # ensure that if self.__connect() fails,
+            # we are not referring to the previous stale connection here
+            self.connection = None
             self.connection = self.__connect()
+
             if self.__pool.dispatch.connect:
                 self.__pool.dispatch.connect(self.connection, self)
         return self.connection

diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py
index 451cb8b0eb265ec01f1fa83a64e59f659501597c..8551e1fcb4613413693c3444e744a7a612efd6c9 100644 (file)
--- a/test/engine/test_pool.py
+++ b/test/engine/test_pool.py
@@ -8,8 +8,9 @@ from sqlalchemy.testing import eq_, assert_raises, is_not_, is_
 from sqlalchemy.testing.engines import testing_engine
 from sqlalchemy.testing import fixtures
 import random
-from sqlalchemy.testing.mock import Mock, call, patch
+from sqlalchemy.testing.mock import Mock, call, patch, ANY
 import weakref
+import collections
 
 join_timeout = 10
 
@@ -1480,6 +1481,98 @@ class QueuePoolTest(PoolTestBase):
         time.sleep(1.5)
         self._assert_cleanup_on_pooled_reconnect(dbapi, p)
 
+    def test_connect_handler_not_called_for_recycled(self):
+        """test [ticket:3497]"""
+
+        dbapi, p = self._queuepool_dbapi_fixture(
+            pool_size=2, max_overflow=2)
+
+        canary = Mock()
+
+        c1 = p.connect()
+        c2 = p.connect()
+
+        c1.close()
+        c2.close()
+
+        dbapi.shutdown(True)
+
+        bad = p.connect()
+        p._invalidate(bad)
+        bad.close()
+        assert p._invalidate_time
+
+        event.listen(p, "connect", canary.connect)
+        event.listen(p, "checkout", canary.checkout)
+
+        assert_raises(
+            Exception,
+            p.connect
+        )
+
+        p._pool.queue = collections.deque(
+            [
+                c for c in p._pool.queue
+                if c.connection is not None
+            ]
+        )
+
+        dbapi.shutdown(False)
+        c = p.connect()
+        c.close()
+
+        eq_(
+            canary.mock_calls,
+            [
+                call.connect(ANY, ANY),
+                call.checkout(ANY, ANY, ANY)
+            ]
+        )
+
+    def test_connect_checkout_handler_always_gets_info(self):
+        """test [ticket:3497]"""
+
+        dbapi, p = self._queuepool_dbapi_fixture(
+            pool_size=2, max_overflow=2)
+
+        c1 = p.connect()
+        c2 = p.connect()
+
+        c1.close()
+        c2.close()
+
+        dbapi.shutdown(True)
+
+        bad = p.connect()
+        p._invalidate(bad)
+        bad.close()
+        assert p._invalidate_time
+
+        @event.listens_for(p, "connect")
+        def connect(conn, conn_rec):
+            conn_rec.info['x'] = True
+
+        @event.listens_for(p, "checkout")
+        def checkout(conn, conn_rec, conn_f):
+            assert 'x' in conn_rec.info
+
+        assert_raises(
+            Exception,
+            p.connect
+        )
+
+        p._pool.queue = collections.deque(
+            [
+                c for c in p._pool.queue
+                if c.connection is not None
+            ]
+        )
+
+        dbapi.shutdown(False)
+        c = p.connect()
+        c.close()
+
+
     def test_error_on_pooled_reconnect_cleanup_wcheckout_event(self):
         dbapi, p = self._queuepool_dbapi_fixture(pool_size=1,
                                         max_overflow=2)