Skip to content

Emit error when backend unexpectedly disconnects #1316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

brianc
Copy link
Owner

@brianc brianc commented Jun 9, 2017

Before this patch when the socket to the PostgreSQL server was terminated unexpectedly the client would emit a close event but not emit an error event. This means all clients in the pool would silently close without a user having any way to respond. This several problems around database failovers, silently "hanging" applications, etc.

fixes #1113 #1075

also possibly fixes #967

I don't think we need to wait for 7.0 for this patch because it is fixing an incorrect behavior which causes silent application failures: it does not change any external APIs...so I think this is a backwards compatible change except sometimes applications which previously silently disconnected without reconnect or became unresponsive will now properly emit errors.

@brianc
Copy link
Owner Author

brianc commented Jun 9, 2017

note: the native bindings behaved correctly and emitted errors on unexpected disconnections - only the pure JS driver needed changing.

@brianc
Copy link
Owner Author

brianc commented Jun 9, 2017

To simulate network partitions and backend disconnections I ended up implementing a fake PostgreSQL Server backend I could run within the integration test.

Once this was implemented I could connect node-postgres clients to it and force terminations of the fake server. After I did this I was able to immediately reproduce the issue of clients 'dying' without emitting an error.

@brianc brianc merged commit 76c59a0 into master Jun 9, 2017
@brianc brianc deleted the fix-network-partitions branch June 9, 2017 17:27
@marshallford
Copy link

This patch fixes the problem described for my app. Thanks!

@okischuang
Copy link

Great efforts @brianc !!

Comparing to this, what I've done is implementing a setTimeout timer binding on connection instance, let it emit an error and then let error be fired through the path now the library already has.
connection -> client -> pool -> pool destroy

what do you think of this solution? is it helpful or appropriate to the issue?

Appreciate for the comments if you would like to give :>

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.

No error emitted on ungraceful db server shutdown LISTEN notifications stop after a while
3 participants