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

diff --git a/doc/build/changelog/unreleased_14/4524.rst b/doc/build/changelog/unreleased_14/4524.rst
new file mode 100644 (file)
index 0000000..409fd19
--- /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 bdb981699ab5613084c68bbae75c6bc03ce9739a..b53f0d7ddb05e08e2b2b83bd58447f8ae5709392 100644 (file)
--- 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 481700e70261b8811257852e51cf14df7a20def6..205c1fb310402c31fbea66f9e33d3da2393afc50 100644 (file)
--- 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(),
+            ],
         )