feat(reconnect): support fresh clients rather than reuse#25
Open
mgagliardo91 wants to merge 1 commit into
Open
feat(reconnect): support fresh clients rather than reuse#25mgagliardo91 wants to merge 1 commit into
mgagliardo91 wants to merge 1 commit into
Conversation
Author
|
@Mikhus hoping you have a moment to review and see if you would be willing to get this patched so we can remove some of the logic on our end to support it |
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.
Problem
When a PostgreSQL connection drops (network blip, server restart, etc.),
pg.Clientcannot be reused — callingconnect()on a previously connected client throws"Client has already been connected. You cannot reuse a client.". This library's built-inreconnect()attempts to callconnect()on the same dead client instance, which always fails.We were seeing that in our production usage of the library and monkey-patched it for now, but are hoping to get this fix into the real library.
We also saw the following behavior:
Error events are re-emitted without the error object: the constructor wires
this.pgClient.on('error', () => this.emit('error')), dropping the error argument entirely. This causes Node.js to throwUnhandled error. (undefined)even when consumers have registered error handlers, since the emitted value isundefinedrather than the actualError.Double reconnect on a single disconnect:
reconnectis registered as aoncehandler on both'error'and'end'viasetOnceHandler. When a connection drops, both events fire, triggering two independent reconnect attempts that race each other.Changes
PgPubSub.tsto replace the pg client on reconnect, re-registering event listeners on the new client insteadTesting
Added some mock pg client behavior to simulate the connect/reconnect and error emitting events
Note: I was unable to get the current test suite running with the version of chai (v5.2.0) due to it being ESM-only, so I downgraded it to
v4which worked. If you are able to run it with v5, just let me know how to do so.