fix(pool): bound each step of returning a connection to the pool#4354
Closed
adriangb wants to merge 1 commit into
Closed
fix(pool): bound each step of returning a connection to the pool#4354adriangb wants to merge 1 commit into
adriangb wants to merge 1 commit into
Conversation
Fixes transact-rs#4349. `Floating::return_to_pool` holds the pool's `DecrementSizeGuard` while it does I/O on the connection: an `after_release` hook, a `ping()` to check the connection is still usable, or a graceful `close()`. None of those were bounded. That matters because the I/O can be on a socket that is dead in a way the socket cannot report. If the server disappeared without closing the connection, and the client has nothing left to retransmit, no RST is ever provoked and reads never complete. The returning task then holds its permit forever and the pool shrinks by one connection. After `max_connections` of those, every `acquire()` fails with `PoolTimedOut` and the pool never recovers, even though the server is back. Each step now gets `RETURN_TO_POOL_TIMEOUT` (5s, matching the existing `CLOSE_ON_DROP_TIMEOUT`), and on expiry the connection is dropped via `close_hard()`, which does no I/O. Cancelling `close()` likewise drops the connection and releases the permit. The regression test uses an `after_release` hook that never completes, which is a deterministic stand-in for a socket that never answers: before this change it exhausts `acquire_timeout`, after it the pool is usable again once the bound expires. I realise transact-rs#3582 reworks this area and includes a timeout of its own; this is meant for main until that lands.
Author
|
Closing in favour of #4350, which does the same thing, was opened first, and is by the author of #4349. I found it only after opening this. Their fake-server test is also a better reproduction than the hanging I have left the one piece of this that #4350 does not cover as a comment there: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4349.
The problem
Floating::return_to_poolholds the pool'sDecrementSizeGuardwhile it does I/O on the connection being returned: theafter_releasehook, theping()that checks the connection is still usable, and the gracefulclose()on the max-lifetime and pool-closed paths. None of those are bounded.That is fine when a connection is either healthy or visibly broken. It is not fine when the socket is dead in a way it cannot report. If the server disappeared without closing the connection (a failover, a killed container, a dropped NAT mapping) and the client has nothing left to retransmit, no RST is ever provoked and the read never completes. The spawned task that is returning the connection then holds its permit forever, and the pool shrinks by one usable connection.
Repeat that
max_connectionstimes and everyacquire()fails withPoolTimedOutfrom then on, even after the server comes back, because the permits are held by tasks parked on dead sockets.We hit this in production behind a maintenance loop that had a query in flight when its Postgres instance went away.
The fix
Each step gets
RETURN_TO_POOL_TIMEOUT(5s, matching the existingCLOSE_ON_DROP_TIMEOUTa few lines up), and on expiry the connection is discarded withclose_hard(), which does no I/O. Cancellingclose()likewise drops the connection and releases the permit, so the bounded-close path is safe too.Test
pool_recovers_from_a_hanging_after_releaseuses anafter_releasehook that never completes, as a deterministic stand-in for a socket that never answers. Onmainit exhaustsacquire_timeoutand fails after 30s; with this change the pool is usable again after the 5s bound:Note on #3582
I saw in #4146 that the new pool architecture in #3582 already covers this with a timeout of its own. This is meant as the fix for
mainuntil that lands, and should be straightforward to drop when it does. Happy to close it if you would rather not carry an interim change.