Skip to content

feat(reconnect): support fresh clients rather than reuse#25

Open
mgagliardo91 wants to merge 1 commit into
imqueue:masterfrom
mgagliardo91:feat-support-fresh-clients
Open

feat(reconnect): support fresh clients rather than reuse#25
mgagliardo91 wants to merge 1 commit into
imqueue:masterfrom
mgagliardo91:feat-support-fresh-clients

Conversation

@mgagliardo91
Copy link
Copy Markdown

Problem

When a PostgreSQL connection drops (network blip, server restart, etc.), pg.Client cannot be reused — calling connect() on a previously connected client throws "Client has already been connected. You cannot reuse a client.". This library's built-in reconnect() attempts to call connect() 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:

  1. 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 throw Unhandled error. (undefined) even when consumers have registered error handlers, since the emitted value is undefined rather than the actual Error.

  2. Double reconnect on a single disconnect: reconnect is registered as a once handler on both 'error' and 'end' via setOnceHandler. When a connection drops, both events fire, triggering two independent reconnect attempts that race each other.

Changes

  • Updates PgPubSub.ts to replace the pg client on reconnect, re-registering event listeners on the new client instead
  • Error emit forwards the error rather than passing a new one

Testing

  • 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 v4 which worked. If you are able to run it with v5, just let me know how to do so.

@mgagliardo91
Copy link
Copy Markdown
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant