Fix promise-based connect() and orphan 'end' events during load-balan…#15
Open
HarshDaryani896 wants to merge 2 commits into
Open
Fix promise-based connect() and orphan 'end' events during load-balan…#15HarshDaryani896 wants to merge 2 commits into
HarshDaryani896 wants to merge 2 commits into
Conversation
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.
…ced failover
Summary
Three fixes to
Clientinpackages/pg/lib/client.jsthat surface when load balancing is enabled and_connect()performs an internal retry/failover to a different host, or when no eligible server can be found.Changes
1.
connect()returns a Promise when no callback is providedThe load-balance branch wraps its work in
lock.acquire().then(...)but never returned that chain, so callingconnect()without a callback returnedundefinedsynchronously. As a resultawait client.connect()did not actually wait for the connection (including any internal retry/failover) to complete.We now wrap the callback-style path in a Promise at the top of
connect(), so both the callback and promise forms behave consistently with upstreampg.2. Ignore
'end'events from orphaned connectionsDuring load-balanced failover,
_connect()may replacethis.connectionwith a newConnectionon a different host. The oldConnection's listeners remain attached, and when its socket eventually fires'end', the regular handler would call_errorAllQueries(): incorrectly rejecting queries that belong to the new, healthy connection.The
'end'handler now detects this case (this.connection !== con) and bails out early, logging atsillylevel.3. Release the lock and surface the error when no server can be found
The load-balance branch returns its work as
lock.acquire().then(...), but a synchronous throw from that path: most commonlygetLeastLoadedServer()raisingError: Could not find a least loaded server.(e.g. every eligible host is stopped), or topology-key parsing: was never handled. The rejection leaked as an unhandled promise rejection while the user callback was never invoked, soawait client.connect()hung forever, and the lock acquired at the top of the branch was never released: deadlocking every subsequentconnect().We now:
returnthelock.acquire().then(...)chain (and the innergetConnection()/getServersInfo()chains) so the failure propagates instead of being dropped, and.catch()that releases the lock and surfaces the error through the callback.connect()now rejects promptly with the underlying error instead of hanging, and the lock is always released.Testing
Ran test for node-postgres driver present in driver-examples repo.