From 061e7f8b62c0f4d37664393985a91cb4b8b0c79a Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 4 Oct 2023 09:42:55 -0400
Subject: [PATCH] check fairy for None in pool cleanup

Fixed issue where under some garbage collection / exception scenarios the
connection pool's cleanup routine would raise an error due to an unexpected
set of state, which can be reproduced under specific conditions.

Fixes: #10414
Change-Id: Ie732f23290d0d3d641f37cd2fee55aff5b9d0857
---
 doc/build/changelog/unreleased_20/10414.rst |  7 +++++
 lib/sqlalchemy/pool/base.py                 |  4 ++-
 test/engine/test_pool.py                    | 32 +++++++++++++++++----
 3 files changed, 36 insertions(+), 7 deletions(-)
 create mode 100644 doc/build/changelog/unreleased_20/10414.rst

diff --git a/doc/build/changelog/unreleased_20/10414.rst b/doc/build/changelog/unreleased_20/10414.rst
new file mode 100644
index 0000000000..6c7159c091
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/10414.rst
@@ -0,0 +1,7 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 10414
+
+    Fixed issue where under some garbage collection / exception scenarios the
+    connection pool's cleanup routine would raise an error due to an unexpected
+    set of state, which can be reproduced under specific conditions.
diff --git a/lib/sqlalchemy/pool/base.py b/lib/sqlalchemy/pool/base.py
index 915dc400b9..90ed32ec27 100644
--- a/lib/sqlalchemy/pool/base.py
+++ b/lib/sqlalchemy/pool/base.py
@@ -1040,7 +1040,9 @@ def _finalize_fairy(
     # test/engine/test_pool.py::PoolEventsTest::test_checkin_event_gc[True]
     # which actually started failing when pytest warnings plugin was
     # turned on, due to util.warn() above
-    fairy.dbapi_connection = fairy._connection_record = None  # type: ignore
+    if fairy is not None:
+        fairy.dbapi_connection = None  # type: ignore
+        fairy._connection_record = None
     del dbapi_connection
     del connection_record
     del fairy
diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py
index e2e4841740..44c494bad4 100644
--- a/test/engine/test_pool.py
+++ b/test/engine/test_pool.py
@@ -738,7 +738,10 @@ class PoolEventsTest(PoolTestBase):
 
     @testing.variation("is_asyncio", [(True, testing.requires.asyncio), False])
     @testing.variation("has_terminate", [True, False])
-    def test_checkin_event_gc(self, is_asyncio, has_terminate):
+    @testing.variation("invalidate_conn_rec", [True, False])
+    def test_checkin_event_gc(
+        self, is_asyncio, has_terminate, invalidate_conn_rec
+    ):
         """tests for #8419, which have been modified for 2.0 in #9237"""
 
         p, canary = self._checkin_event_fixture(
@@ -751,7 +754,11 @@ class PoolEventsTest(PoolTestBase):
 
         eq_(canary, [])
 
-        if is_asyncio:
+        if invalidate_conn_rec:
+            # test #10414
+            c1._connection_record.invalidate()
+
+        if is_asyncio and not invalidate_conn_rec:
             if has_terminate:
                 with expect_warnings(
                     "The garbage collector is trying to clean up.*which will "
@@ -772,7 +779,9 @@ class PoolEventsTest(PoolTestBase):
 
         detach_gced = is_asyncio
 
-        if detach_gced:
+        if invalidate_conn_rec:
+            eq_(canary, ["checkin"])
+        elif detach_gced:
             if has_terminate:
                 eq_(canary, ["reset_no_rollback", "detach", "close_detached"])
             else:
@@ -784,7 +793,7 @@ class PoolEventsTest(PoolTestBase):
             eq_(canary, ["reset_rollback_ok", "checkin"])
 
         gc_collect()
-        if detach_gced:
+        if detach_gced or invalidate_conn_rec:
             is_none(dbapi_connection())
         else:
             is_not_none(dbapi_connection())
@@ -1708,8 +1717,11 @@ class QueuePoolTest(PoolTestBase):
         "detach_gced",
         [("detached_gc", testing.requires.asyncio), "normal_gc"],
     )
+    @testing.variation("invalidate_conn_rec", [True, False])
     @testing.emits_warning("The garbage collector")
-    def test_userspace_disconnectionerror_weakref_finalizer(self, detach_gced):
+    def test_userspace_disconnectionerror_weakref_finalizer(
+        self, detach_gced, invalidate_conn_rec
+    ):
         dbapi, pool = self._queuepool_dbapi_fixture(
             pool_size=1, max_overflow=2, _is_asyncio=detach_gced
         )
@@ -1740,6 +1752,10 @@ class QueuePoolTest(PoolTestBase):
         )
 
         not_closed_dbapi_conn = conn.dbapi_connection
+
+        if invalidate_conn_rec:
+            conn._connection_record.invalidate()
+
         del conn
 
         gc_collect()
@@ -1752,7 +1768,11 @@ class QueuePoolTest(PoolTestBase):
             # this creates a gc-level warning that is not easy to pin down,
             # hence we use the testing.emits_warning() decorator just to squash
             # it
-            eq_(not_closed_dbapi_conn.mock_calls, [call.rollback()])
+
+            if invalidate_conn_rec:
+                eq_(not_closed_dbapi_conn.mock_calls, [call.close()])
+            else:
+                eq_(not_closed_dbapi_conn.mock_calls, [call.rollback()])
 
     @testing.requires.timing_intensive
     def test_recycle_pool_no_race(self):
-- 
2.47.3