From f78db5e1f68d6b2fb6a7acc04036f682d9a22974 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 5 Mar 2019 15:37:00 -0500 Subject: [PATCH] Don't call pre_ping for fresh connection The pool "pre-ping" feature has been refined to not invoke for a DBAPI connection that was just opened in the same checkout operation. pre ping only applies to a DBAPI connection that's been checked into the pool and is being checked out again. Fixes: #4524 Change-Id: Ibe3dfb709dbdc24aa94e96513cfbea456c33b895 --- doc/build/changelog/unreleased_14/4524.rst | 8 ++ lib/sqlalchemy/pool/base.py | 32 +++++--- test/engine/test_reconnect.py | 86 +++++++++++++++++++++- 3 files changed, 111 insertions(+), 15 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/4524.rst diff --git a/doc/build/changelog/unreleased_14/4524.rst b/doc/build/changelog/unreleased_14/4524.rst new file mode 100644 index 0000000000..409fd198e4 --- /dev/null +++ b/doc/build/changelog/unreleased_14/4524.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: feature, pool + :tickets: 4524 + + The pool "pre-ping" feature has been refined to not invoke for a DBAPI + connection that was just opened in the same checkout operation. pre ping + only applies to a DBAPI connection that's been checked into the pool + and is being checked out again. diff --git a/lib/sqlalchemy/pool/base.py b/lib/sqlalchemy/pool/base.py index bdb981699a..b53f0d7ddb 100644 --- a/lib/sqlalchemy/pool/base.py +++ b/lib/sqlalchemy/pool/base.py @@ -360,6 +360,8 @@ class _ConnectionRecord(object): self.__connect(first_connect_check=True) self.finalize_callback = deque() + fresh = False + fairy_ref = None starttime = None @@ -574,6 +576,7 @@ class _ConnectionRecord(object): connection = pool._invoke_creator(self) pool.logger.debug("Created new connection %r", connection) self.connection = connection + self.fresh = True except Exception as e: pool.logger.debug("Error on connect(): %s", e) raise @@ -699,7 +702,6 @@ class _ConnectionFairy(object): if fairy.connection is None: raise exc.InvalidRequestError("This connection is closed") fairy._counter += 1 - if ( not pool.dispatch.checkout and not pool._pre_ping ) or fairy._counter != 1: @@ -712,22 +714,30 @@ class _ConnectionFairy(object): # here. attempts = 2 while attempts > 0: + connection_is_fresh = fairy._connection_record.fresh + fairy._connection_record.fresh = False try: if pool._pre_ping: - if fairy._echo: - pool.logger.debug( - "Pool pre-ping on connection %s", fairy.connection - ) - - result = pool._dialect.do_ping(fairy.connection) - if not result: + if not connection_is_fresh: if fairy._echo: pool.logger.debug( - "Pool pre-ping on connection %s failed, " - "will invalidate pool", + "Pool pre-ping on connection %s", fairy.connection, ) - raise exc.InvalidatePoolError() + result = pool._dialect.do_ping(fairy.connection) + if not result: + if fairy._echo: + pool.logger.debug( + "Pool pre-ping on connection %s failed, " + "will invalidate pool", + fairy.connection, + ) + raise exc.InvalidatePoolError() + elif fairy._echo: + pool.logger.debug( + "Connection %s is fresh, skipping pre-ping", + fairy.connection, + ) pool.dispatch.checkout( fairy.connection, fairy._connection_record, fairy diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py index 481700e702..205c1fb310 100644 --- a/test/engine/test_reconnect.py +++ b/test/engine/test_reconnect.py @@ -18,6 +18,7 @@ from sqlalchemy.testing import engines from sqlalchemy.testing import eq_ from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures +from sqlalchemy.testing import is_ from sqlalchemy.testing import is_false from sqlalchemy.testing import is_true from sqlalchemy.testing import mock @@ -149,7 +150,7 @@ class PrePingMockTest(fixtures.TestBase): def setup(self): self.dbapi = MockDBAPI() - def _pool_fixture(self, pre_ping): + def _pool_fixture(self, pre_ping, pool_kw=None): dialect = url.make_url( "postgresql://foo:bar@localhost/test" ).get_dialect()() @@ -158,6 +159,7 @@ class PrePingMockTest(fixtures.TestBase): creator=lambda: self.dbapi.connect("foo.db"), pre_ping=pre_ping, dialect=dialect, + **(pool_kw if pool_kw else {}) ) dialect.is_disconnect = lambda e, conn, cursor: isinstance( @@ -168,6 +170,66 @@ class PrePingMockTest(fixtures.TestBase): def teardown(self): self.dbapi.dispose() + def test_ping_not_on_first_connect(self): + pool = self._pool_fixture( + pre_ping=True, pool_kw=dict(pool_size=1, max_overflow=0) + ) + + conn = pool.connect() + dbapi_conn = conn.connection + eq_(dbapi_conn.mock_calls, []) + conn.close() + + # no ping, so no cursor() call. + eq_(dbapi_conn.mock_calls, [call.rollback()]) + + conn = pool.connect() + is_(conn.connection, dbapi_conn) + + # ping, so cursor() call. + eq_(dbapi_conn.mock_calls, [call.rollback(), call.cursor()]) + + conn.close() + + conn = pool.connect() + is_(conn.connection, dbapi_conn) + + # ping, so cursor() call. + eq_( + dbapi_conn.mock_calls, + [call.rollback(), call.cursor(), call.rollback(), call.cursor()], + ) + + conn.close() + + def test_ping_not_on_reconnect(self): + pool = self._pool_fixture( + pre_ping=True, pool_kw=dict(pool_size=1, max_overflow=0) + ) + + conn = pool.connect() + dbapi_conn = conn.connection + conn_rec = conn._connection_record + eq_(dbapi_conn.mock_calls, []) + conn.close() + + conn = pool.connect() + is_(conn.connection, dbapi_conn) + # ping, so cursor() call. + eq_(dbapi_conn.mock_calls, [call.rollback(), call.cursor()]) + + conn.invalidate() + + is_(conn.connection, None) + + # connect again, make sure we're on the same connection record + conn = pool.connect() + is_(conn._connection_record, conn_rec) + + # no ping + dbapi_conn = conn.connection + eq_(dbapi_conn.mock_calls, []) + def test_connect_across_restart(self): pool = self._pool_fixture(pre_ping=True) @@ -242,7 +304,17 @@ class PrePingMockTest(fixtures.TestBase): old_dbapi_conn = conn.connection conn.close() - eq_(old_dbapi_conn.mock_calls, [call.cursor(), call.rollback()]) + # no cursor() because no pre ping + eq_(old_dbapi_conn.mock_calls, [call.rollback()]) + + conn = pool.connect() + conn.close() + + # connect again, we see pre-ping + eq_( + old_dbapi_conn.mock_calls, + [call.rollback(), call.cursor(), call.rollback()], + ) self.dbapi.shutdown("execute", stop=True) self.dbapi.restart() @@ -253,13 +325,19 @@ class PrePingMockTest(fixtures.TestBase): gc_collect() # new connection was reset on return appropriately - eq_(dbapi_conn.mock_calls, [call.cursor(), call.rollback()]) + 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.cursor(), call.rollback(), call.cursor(), call.close()], + [ + call.rollback(), + call.cursor(), + call.rollback(), + call.cursor(), + call.close(), + ], ) -- 2.39.5