fix: double client.end() hang - #2717

The await on the line above should take care of this, right?

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.

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.

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

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

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.