Skip to content

Ensure connect callback is invoked on premature socket hangup #546

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 6, 2014
Merged

Ensure connect callback is invoked on premature socket hangup #546

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
64d6883
Ensure connect callback is invoked on premature socket hangup
Mar 17, 2014
e192358
Add unit test for callback on early postgresql disconnect
Mar 18, 2014
d21b995
Enable the test for #534 (needs special naming)
Mar 18, 2014
96e4afd
Have Connection also emit 'end' on stream 'close' event
Mar 18, 2014
e1b1c62
Do not emit 'end' twice from Connection on close
Apr 4, 2014
e72aff4
Have stream emit 'close' rather than 'end' for sake of testing
Apr 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,17 @@ Client.prototype.connect = function(callback) {
return self.emit('error', error);
}
callback(error);
callback = null;
});

con.once('end', function() {
if ( callback ) {
// haven't received a connection message yet !
var err = new Error("Stream unexpectedly ended before getting ready for query execution");
callback(err);
callback = null;
return;
}
if(self.activeQuery) {
var disconnectError = new Error('Stream unexpectedly ended during query execution');
self.activeQuery.handleError(disconnectError);
Expand Down
7 changes: 7 additions & 0 deletions lib/connection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ Connection.prototype.connect = function(port, host) {
self.emit('end');
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// node-0.10 emits both 'end' and 'close'
// for streams closed by the peer
self.emit('end');
});

if(!this.ssl) {
return this.attachListeners(this.stream);
}
Expand Down
22 changes: 22 additions & 0 deletions test/unit/client/early-disconnect-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
var helper = require(__dirname + '/test-helper');
var net = require('net');
var pg = require('../../..//lib/index.js');

var server = net.createServer(function(c) {
console.log('server connected');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome test! Love this. 💙

c.destroy();
console.log('server socket destroyed.');
server.close(function() { console.log('server closed'); });
});

server.listen(7777, function() {
console.log('server listening');
var client = new pg.Client('postgres://localhost:7777');
console.log('client connecting');
client.connect(assert.calls(function(err) {
if (err) console.log("Error on connect: "+err);
else console.log('client connected');
assert(err);
}));

});