Ensure connect callback is invoked on premature socket hangup

[strk]

Closes #534
Applies to 2.11.1, only really fixes the case with node-0.10, 0.8 is still being researched upon.
Deprecates pull #542

[strk]

Confirmation of this change fixing the bug for node-0.10 but not for node-0.8: https://travis-ci.org/brianc/node-postgres/builds/21012687

Looking at the node-0.8 case next

[strk]

So it turns out node-0.8 does not send the 'end' stream event on backend close.
Here the trace with node-0.10:

server listening
client connecting
server connected
server socket destroyed.
server closed
got stream 'connect'
got stream 'end'
Error on connect: Error: Stream unexpectedly ended before getting ready for query execution

Here the trace with node-0.8:

server listening
client connecting
got stream 'connect'
server connected
server socket destroyed.
server closed

[strk]

Got it: node-0.8 emits 'close' !

[strk]

Catching "close" fixes if for node-0.8 too: https://travis-ci.org/brianc/node-postgres/builds/21015203

[brianc]

This is great! Thanks! I'll get this tested & merged in ASAP.

[brianc]

is this TODO still needed? did you already fix this for v0.8.x and v0.10.x?

[strk]

On Sat, Mar 22, 2014 at 08:50:57AM -0700, Brian C wrote:

@@ -60,6 +60,13 @@ Connection.prototype.connect = function(port, host) {
self.emit('end');
});

• this.stream.on('close', function() {
• // TODO: avoid emitting 'end' twice ?

is this TODO still needed? did you already fix this for v0.8.x and v0.10.x?

My problem was fixed and tested with both v0.8 and v0.10

The TODO is needed as it asks for an opinion from you :)
You see that both "end" and "close" events trigger "end".
Do you think that's a problem ?
Node-0.8 only sends "close".

client.js effects on 'end' are all protected by double-call
except for further sending 'end' to upper levels.

the pool calls 'destroy' and receiving 'end' from client but
is protected by the double call with this:

      // Remove connection from pool on disconnect
      client.on('end', function(e) {
        // Do not enter infinite loop between pool.destroy
        // and client 'end' event...
        if ( ! client._destroying ) {
          pool.destroy(client);
        }
      });

So is not a problem, but you migt not like the double 'end'
event anyway.

--strk;

[brianc]

Thanks for the explanation! Yeah, definitely emitting end twice would be something to avoid. I don't think it should be too hard, just have a like 'this._ended' flag or something on the client, set it the first time, and check if it's set & don't emit. What you think?

[strk]

Sure, that's ok for me, but I'm going to do it myself, do you mind ?

I've seen it used already for the connection object, so maybe you could
reuse an existing flag or something like that... It was called _ending
or _closing or something similar...

[tjfontaine]

In v0.10 and forward 'end' means the readable stream has received EOF, 'close' means the underlying resource (socket/fd) has been closed. It should only be necessary to listen to 'close'.

[strk]

Thank you. It should be tested with node-0.8 if only listening for "close" is ok.
I guess we'd still need to emit "end" to maintain compatibility in the higher levels.

[brianc]

I think only listening for close is okay. As long as we don't emit end twice and it works on v0.8 and v0.10 I'm cool with it. Lemme know when you're ready for another 🔍 and hopefully a merge!

[strk]

I've pushed a commit for only listening on 'close'. Tested locally with node 0.8.9, 0.8.25 and 0.10.26

[brianc]

Hmm....with the new commit travis tests are failing. :(

[strk]

Oops, my testing was bogus as I assumed all failures were being shown while first (unrelated) failure was interrupting the continuation of suite.

[strk]

The failing test "simulates" a stream emitting an 'end' event, so 'close' is never sent.
I could change the test so it emits 'close' instead, but can we be sure that all streams always send 'close' on end with all node versions ?

[brianc]

Hey thanks for your patience on all the back and forth on this! I'll get it merged in today & doing a new release with it & a bunch of other changes. It's long over-due! 😄 👍