fix: double client.end() hang

[cody-greene]

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 ✔

[charmander]

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)

[cody-greene]

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.

[charmander]

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.

[cody-greene]

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

[charmander]

Yeah, connection emitting end twice doesn’t seem good. Not sure if it’s intended.

[cody-greene]

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.

[brianc]

Hey sorry for the headache here but CI was broken. I fixed it yesterday...would you be able to rebase onto master? Then the tests should pass and we can merge this up and get a release out! Thank you so much for contributing! ❤️

[cody-greene]

Is anything else needed here?

[MasterOdin]

I tested this PR and it will also fix the issue I just reported in #2777. 👍

[cody-greene]

Can I do anything else to help get this merged, @brianc ?

[mastermatt]

This will also fix #2923.
@brianc as today is the one year anniversary for this PR, could we get you to take a fresh look at the change?

[brianc]

as today is the one year anniversary for this PR, could we get you to take a fresh look at the change?

haha yes I'll get this released today 😄

[cody-greene]

[brianc]

haha yeah sorry it took so long...sometimes things get buried for me in my inbox and go bye bye - appreciate it deeply though! ❤️