integrate asyncpg terminate into pool finalize fairy, invalidation cases

Discussed in #8418

we can resolve the issue from #8145 using asyncpg.terminate().

additionally: any pool recycling of a connection that is known to be invalid, or may be invalid, should also use terminate, as asyncpg is reportedly hanging for LB connection that has been dropped. so it's a bug to be fixed. not sure about 1.4 feasibility, will have to see how it proceeds.

dialect: gets new do_terminate() method

asyncpg dialect: implements do_terminate()

pool finalize fairy: instead of checking for just "is_async", has to check "is_astync and not has_terminate". if has_terminate is set, then we can proceed.

excerpt

diff --git a/lib/sqlalchemy/pool/base.py b/lib/sqlalchemy/pool/base.py
index 51eb5e9f5e..5c68f9d3b0 100644
--- a/lib/sqlalchemy/pool/base.py
+++ b/lib/sqlalchemy/pool/base.py
@@ -310,6 +310,15 @@ class Pool(log.Identified, event.EventTarget):
             creator_fn = cast(_CreatorFnType, creator)
             return lambda rec: creator_fn()
 
+    def _terminate_connection(self, connection: DBAPIConnection) -> None:
+        self.logger.debug("Hard-closing connection %r", connection)
+        try:
+            self._dialect.do_terminate(connection)
+        except Exception:
+            self.logger.error(
+                "Exception hard-closing connection %r", connection, exc_info=True
+            )
+
     def _close_connection(self, connection: DBAPIConnection) -> None:
         self.logger.debug("Closing connection %r", connection)
         try:
@@ -877,7 +886,7 @@ def _finalize_fairy(
         dbapi_connection = connection_record.dbapi_connection
 
     # null pool is not _is_asyncio but can be used also with async dialects
-    dont_restore_gced = pool._dialect.is_async
+    dont_restore_gced = pool._dialect.is_async and not pool._dialect.has_terminate
 
     if dont_restore_gced:
         detach = connection_record is None or is_gc_cleanup
@@ -918,7 +927,7 @@ def _finalize_fairy(
                     if pool.dispatch.close_detached:
                         pool.dispatch.close_detached(dbapi_connection)
 
-                    pool._close_connection(dbapi_connection)
+                    pool._terminate_connection(dbapi_connection)
                 else:
                     message = (
                         "The garbage collector is trying to clean up "

Mike Bayer has proposed a fix for this issue in the main branch:

integrate connection.terminate() for supporting dialects https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4045

Mike Bayer has proposed a fix for this issue in the rel_1_4 branch:

integrate connection.terminate() for supporting dialects https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4046

@skrech

@skrech can you test the 1.4 patch at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4046 ? we have limited ability to simulate your conditions so being able to test the implementation here that it solves the timeout issue is important.

Thanks for the quick fix! Will definitely try it out!
It'll take me some time though, because the LB timeout is long.

I've read the source code for asyncpg in this regard and both .close() and .terminate() ultimately both send the "terminate" command, "X", to the server. .close() waits for the server to drop the connection, .terminate() does not.

the other thing .close() does is cancel any statements that are still running. this doesnt apply to our pool case too much if at all. but definitely for the garbage collection case as well as connections that are being recycled or invalidated, we should be calling terminate.

Trying out a simple pg_terminate_backend() case, asyncpg doesn't have an issue here. so this is specific to how the LB is dropping the connection.

FWIW I've dealt a lot with haproxy and mysql and we have also seen issues like this for sure. The biggest hang we would get is if the VIP of haproxy would change, the clients had zero idea anything had changed and they'd hang for a very long time.

I can confirm the issue and that the fix works by stopping the network before closing a connection. test case:

import asyncio
import time

from sqlalchemy.ext.asyncio import create_async_engine


async def async_main():
    # engine is an instance of AsyncEngine
    engine = create_async_engine(
        "postgresql+asyncpg://scott:tiger@pg14/test",
        echo=True,
        echo_pool="debug",
        pool_recycle=5,
    )

    conn = await engine.connect()


    await conn.close()

    time.sleep(5)

    print(
        "returned the connection to the pool.\n"
        "Now, stop the network.  then press any key to continue.\n"
        "It should immediately close the old "
        "connection, try to connect again, and give an immediate network is "
        "unreachable error.  if the bug, it should hang on trying to close "
        "the connection that's to be recycled."
    )
    input("stop the network and then press any key to continue.")

    conn = await engine.connect()
    await conn.close()


#    print("closed")

asyncio.run(async_main())

you need to test on a database that's over the network, then disable your network adapter in order to simulate a black hole connection.

I've setup a test environment, will come back with results tomorrow after 10:00 GMT.

integrate connection.terminate() for supporting dialects

I confirm that the fix is working!

thats great. yeah I was able to reproduce it here completely.

Navigation Menu

Repository navigation

integrate asyncpg terminate into pool finalize fairy, invalidation cases #8419

Discussed in #8418

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

integrate asyncpg terminate into pool finalize fairy, invalidation cases #8419

Description

Discussed in #8418

Activity

sqla-tester commented on Aug 23, 2022

sqla-tester commented on Aug 23, 2022

zzzeek commented on Aug 23, 2022

skrech commented on Aug 23, 2022

zzzeek commented on Aug 23, 2022

zzzeek commented on Aug 23, 2022

zzzeek commented on Aug 23, 2022

zzzeek commented on Aug 23, 2022

skrech commented on Aug 24, 2022

skrech commented on Aug 25, 2022

zzzeek commented on Aug 25, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions