The Wayback Machine - https://web.archive.org/web/20220402194121/https://github.com/brianc/node-postgres/pull/2717
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

fix: double client.end() hang #2717

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cody-greene
Copy link
Contributor

@cody-greene cody-greene commented Mar 7, 2022

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 makes client.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

Emitted when the other end of the socket signals the end of
transmission, thus ending the readable side of the socket.

https://nodejs.org/docs/latest-v16.x/api/net.html#event-close_1

Emitted once the socket is fully closed.

test before change:

2716-tests.js
  client.end() should resolve if already ended
Error: test: client.end() should resolve if already ended did not complete withint 5000ms
    at Timeout._onTimeout (/home/cody/repos/node-postgres/packages/pg/test/suite.js:52:19)
    at listOnTimeout (node:internal/timers:557:17)
    at processTimers (node:internal/timers:500:7)

test after change:

2716-tests.js
  client.end() should resolve if already ended ✔

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)
Copy link
Collaborator

@charmander charmander Mar 7, 2022

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?

Suggested change
// 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)

Copy link
Contributor Author

@cody-greene cody-greene Mar 7, 2022

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.

Copy link
Collaborator

@charmander charmander Mar 7, 2022

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.

Copy link
Contributor Author

@cody-greene cody-greene Mar 7, 2022

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

Copy link
Collaborator

@charmander charmander Mar 7, 2022

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.

Copy link
Contributor Author

@cody-greene cody-greene Mar 7, 2022

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.

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.

2 participants