pool finalize calls rollback for async in gc handler when can_manipulate_connection = FAlse

[zzzeek]

Discussed in #9235

TODO: add test case here, see discussion

this may be backportable to 1.4 but it might not be worth it as some of pooling code has moved around in 2.0

[zzzeek]

so we dont normally see this happen because we also added shield for #8145 in 1acaf0b. I can make the test case in #8145 either emit the user's original warning, or show the bad stack trace we want to fix here, by removing the shield() calls and then either removing support for terminate, or putting terminate support back in.

that is, to see the really bad exception, remove the shield calls from asyncio/engine.py alone and then run the test case at the top of #8145, leaving in the pool's "terminate" logic.

so here we will seek to continue to not call "rollback" if we are asyncio and are in gc.

[zzzeek]

also major issue with #8145 is that we didnt add real asyncio tests. not sure why not.

[zzzeek]

this whole thing is a little mystifying. the pool does not even call terminate in finalize_fairy at all. like it's trying to return the async connection to the pool. it's also extremely difficult to test any of this.

[sqla-tester]

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

do not return asyncio connections to the pool under gc https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4428

[zzzeek]

so issue #8419 is absolutely a regression in how this situation is handled, while it likely improves the explicit invalidate() scenario which is why it should be kept in place, it causes asyncpg connections under GC to actually have .rollback() called on them and then be returned to the pool if that succeeded. This "works" because the greenlet thing works out. if the operation occurs within the gc background thread or in shutdown, we get lots of stack traces. the pool remains working reliably but the error conditions for connection not returned are much more of a mess.

therefore for now I dont want to backport the fix here as it is a significant change; correctly written asyncio apps will continue to work fine under 1.4 and 2.0.

[CaselIT]

So asyncio is hard and behaves in strange and hard to reason about ways in complex situation. News at 11.

Is a terminate like method something we should ask other async driver to provide? Like I don't think psycopg3 has something similar

[zzzeek]

I dont think terminate is that important here and this mostly had to do with doing things "correctly" re: connection.invalidate() for #8419.

the issue here is that we are hitting asyncio connections with GC and returning them to the pool, within the gc handler, including rollback on return, and it just happens to work when the gc is synchronous. That's why #9235 has a huge amount of stack trace, because its doing the rollback + close in the gc cleanup which is outside of the asyncio event loop.

with the fix here, the given test case emits the warning and then does the terminate, which still logs a stack trace, but the warning is first and loud and clear, and there's no attempt to rollback() or anything like that.

[CaselIT]

I dont think terminate is that important here and this mostly had to do with doing things "correctly" re: connection.invalidate() for #8419.

I meant in general, not particularly in connection with this issue