From: Mike Bayer Date: Wed, 22 Jul 2015 21:59:34 +0000 (-0400) Subject: - Fixed critical issue whereby the pool "checkout" event handler X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0aee8eb0015b2fa2dac0d2ba9d781fd2663bf042;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - 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 (cherry picked from commit 4096ad0f0980f6940be57aaee85791df8f975cd7) --- diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index 055dea80d4..9bbadee65c 100644 --- 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/__init__.py b/lib/sqlalchemy/__init__.py index e6d589f6de..0f65e31883 100644 --- a/lib/sqlalchemy/__init__.py +++ b/lib/sqlalchemy/__init__.py @@ -117,7 +117,7 @@ from .schema import ( from .inspection import inspect from .engine import create_engine, engine_from_config -__version__ = '0.9.10' +__version__ = '0.9.11' def __go(lcls): diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py index 54cc13c75a..0701402346 100644 --- a/lib/sqlalchemy/pool.py +++ b/lib/sqlalchemy/pool.py @@ -527,6 +527,9 @@ class _ConnectionRecord(object): if recycle: self.__close() + # ensure that if self.__connect() fails, + # we are not referring to the previous stale connection here + self.connection = None self.connection = self.__connect() self.info.clear() if self.__pool.dispatch.connect: diff --git a/lib/sqlalchemy/testing/mock.py b/lib/sqlalchemy/testing/mock.py index be83693cc9..c836bb407b 100644 --- a/lib/sqlalchemy/testing/mock.py +++ b/lib/sqlalchemy/testing/mock.py @@ -11,10 +11,10 @@ from __future__ import absolute_import from ..util import py33 if py33: - from unittest.mock import MagicMock, Mock, call, patch + from unittest.mock import MagicMock, Mock, call, patch, ANY else: try: - from mock import MagicMock, Mock, call, patch + from mock import MagicMock, Mock, call, patch, ANY except ImportError: raise ImportError( "SQLAlchemy's test suite requires the " diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py index e84f8803a4..6fd25290d1 100644 --- 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_ from sqlalchemy.testing.engines import testing_engine from sqlalchemy.testing import fixtures import random -from sqlalchemy.testing.mock import Mock, call +from sqlalchemy.testing.mock import Mock, call, ANY import weakref +import collections join_timeout = 10 @@ -1362,6 +1363,97 @@ class QueuePoolTest(PoolTestBase): time.sleep(1) 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_recycle_pool_no_race(self): def slow_close(): slow_closing_connection._slow_close()