Skip to content
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

Client.end never resolves if the socket was already destroyed #2923

Closed
mastermatt opened this issue Mar 2, 2023 · 4 comments
Closed

Client.end never resolves if the socket was already destroyed #2923

mastermatt opened this issue Mar 2, 2023 · 4 comments

Comments

@mastermatt
Copy link

I've been debugging this issue kibae/pg-logical-replication#20 and narrowed it down to the Client's end method not resolving (or calling the callback) under certain error conditions.

Minimal reproduction:

const client = new Client(connOpts)
await client.connect()

setImmediate(() => {
  client.connection.stream.resetAndDestroy() // simulate a network outage or the db going offline e.g. ECONNRESET

  setImmediate(async () => {
    await client.end() // <-- never resolves
  })
})

Notably, by the time .end is called, the connection and its stream (a new.Socket instance) have already emitted their close and end events. However, .end relies on an end event from the connection to resolve, unless the connection never connected to begin with.
I believe the check to have .end resolve early should not only look at the _connecting flag, but should also check if the socket has been destroyed.

// if we have never connected, then end is a noop, callback immediately
if (!this.connection._connecting) {

I believe these issues are related:

  1. Connection hang on the server side after ended #2329
  2. Bug in pool.end () never returns when trying to close cleanly #2341
  3. pg version 8.7.1 hangs on await db.end() but before version 8 doesn't #2648
@fmmoret
Copy link

fmmoret commented Mar 4, 2023

Timing is uncanny -- ran into this as well today on "pg": "8.7.1"

@fmmoret
Copy link

fmmoret commented Mar 4, 2023

Uncertain but this open PR might do it: https://github.com/brianc/node-postgres/pull/2717/files

@mastermatt
Copy link
Author

@fmmoret yes, it would fix this. I'm surprised I didn't come across that one when I was searching for existing issues. The cause is described differently, but it's the same underlaying issues of the stream having already emitted its 'close' event.

@mastermatt
Copy link
Author

My local testing shows this was resolved with 8.10.0.

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

No branches or pull requests

2 participants