fix(pg-pool): honor connectionTimeoutMillis and don't leak timed-out connections#3711
Open
malbolged wants to merge 2 commits into
Open
fix(pg-pool): honor connectionTimeoutMillis and don't leak timed-out connections#3711malbolged wants to merge 2 commits into
malbolged wants to merge 2 commits into
Conversation
…connections When connectionTimeoutMillis fired while a new connection was still being established, the connect callback's success path never checked whether the timeout had already fired. If the connection finished establishing just after the deadline, the pool would resolve the checkout successfully - silently violating connectionTimeoutMillis - and retain a connection it had already given up on. The timeout callback also tore down the local socket with stream.destroy() instead of terminating the connection cleanly. This changes newClient() to: - reject the checkout at the deadline and remove the client from _clients immediately, so a connect that hangs (or never calls back) can neither delay the timeout nor leak into _clients - terminate the half-open connection cleanly (connection.end() sends a proper Terminate) instead of destroying the local socket - silence the dying client's 'error' events before ending it, so they don't surface as an unhandled 'error' (which can crash the process) - discard, rather than hand back, a connection that finishes establishing after the timeout already rejected the checkout Adds a deterministic regression test (a 200ms connect against a 100ms timeout) that fails on the previous behavior and passes with this change. Fixes brianc#3543
Owner
|
Write the PR description yourself and dont submit a huge wall of text slop from an agent. Getting a 55 line change PR is fine but not if it is accompanied by a thousand line, zero effort PR description. |
The previous commit hoisted `timeoutHit = true` and the checkout rejection above the `if (client.connection) / else if (!client.isConnected())` check, which dropped an intentional behavior: a client with no connection object that already reports `isConnected()` (the native client) was never timed out, because it has connected and simply hasn't invoked its connect callback yet. Restore that exemption by returning early from the timeout callback in that case, which fixes `should connect if timeout is passed, but native client in connected state`.
Author
I'm sorry, I asked claude to generate a PR description because I didn't want to manually write it and it generated a wall of slop that made no sense, I apologize for that. PS: First and last time I ask it to generate PR description, I also asked it to generate the unit test. |
Owner
|
Its okay! I was a bit spicy last night after a long work week. As a maintainer the low effort slop PRs start to beat me down. |
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
2 participants
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 #3543.
newClient()sets aconnectionTimeoutMillistimer, but theclient.connect()success path never checks whether it fired. A connection that finishes establishing after the deadline is handed to the caller anyway — silently exceeding the timeout — and kept in the pool.The timeout now rejects the checkout at the deadline, drops the client, and ends the connection cleanly rather than destroying the socket. A connection that completes after the timeout is closed instead of handed back. A client that reports
isConnected()with noconnectionobject (the native client) is still exempt, as before.