brianc / node-postgres Public
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
fix: double client.end() hang #2717
base: master
Are you sure you want to change the base?
Conversation
fixes brianc#2716 `client.end()` will resolve early if the connection is already dead, rather than waiting for an "end" event that will never arrive.
// connection "end" event is emitted twice; once on stream "close" and once | ||
// on stream "end" so we need to make sure our second client.end() is not | ||
// resolved by these. | ||
await sleep(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The await
on the line above should take care of this, right?
// connection "end" event is emitted twice; once on stream "close" and once | |
// on stream "end" so we need to make sure our second client.end() is not | |
// resolved by these. | |
await sleep(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it does not. There seems so be some process.nextTick() shenanigans which means that with just:
await client.end()
await client.end()
The 2nd end() is actually resolved as a result of events emitted from the first end() and the test will succeed without code changes. Adding the sleep() call makes the test actually fail before the code change, and then pass with the code change. If that still doesn't make sense then I can try to point to the source with more detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do. It looks to me like end
would either have to be emitted twice on the connection for that to happen (bad), or the connection’s _connecting
would have to transition back to false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To further clarify: the "end" event which resolves the client.end()
promise/callback is emitted when client.connection.stream
emits "end" itself i.e. the readable side of the socket is closed, and again when the stream emits "close". The close event may not be emitted on the same tick, and could be several ticks later, basically whenever the socket is fully and finally closed.
So I could actually write this test with 3 end()
s rather than a sleep()
await client.end()
// connection.end()
// stream.end()
// ...
// stream emits "end"
// connection emits "end"
// promise resolved
await client.end()
// connection.end()
// stream.end() but this is already closing and does nothing
// ...
// stream emits "close"; no more events will be emitted from the stream
// connection emits "end"
// promise resolved
await client.end() // <-- this will wait forever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, connection emitting end
twice doesn’t seem good. Not sure if it’s intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was initially expecting the first client.end()
to resolve only when the underlying socket is fully closed i.e. stream.on('close')
. At this point I'm wondering if that's something I should try to change as well. Reason being: resolving before the client is really dead is technically incorrect.
fixes #2716
client.end()
will resolve early if the connection is already dead, rather than waiting for an "end" event that will never arrive. This also makesclient.end()
resolve only when the underlying socket is fully closed, both the readable part ("end" event) & writable part ("close" event).https://nodejs.org/docs/latest-v16.x/api/net.html#event-end
https://nodejs.org/docs/latest-v16.x/api/net.html#event-close_1
test before change:
test after change: