Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
cleartextstream.destroy() should destroy socket.
Browse files Browse the repository at this point in the history
This fixes a critical bug see in MJR's production. Very difficult to build a
test case. Sometimes HTTPS server gets sockets that are hanging in a
half-duplex state.
  • Loading branch information
ry committed May 2, 2011
1 parent 82bc25d commit 75a0cf9
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 8 deletions.
22 changes: 20 additions & 2 deletions lib/stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ Stream.prototype.pipe = function(dest, options) {
dest._pipeCount++;

source.on('end', onend);
source.on('close', onend);
source.on('close', onclose);
}

var didOnEnd = false;
Expand All @@ -74,6 +74,24 @@ Stream.prototype.pipe = function(dest, options) {
dest.end();
}


function onclose() {
if (didOnEnd) return;
didOnEnd = true;

dest._pipeCount--;

// remove the listeners
cleanup();

if (dest._pipeCount > 0) {
// waiting for other incoming streams to end.
return;
}

dest.destroy();
}

// don't leave dangling pipes when there are errors.
function onerror(er) {
cleanup();
Expand Down Expand Up @@ -117,7 +135,7 @@ Stream.prototype.pipe = function(dest, options) {
dest.removeListener('drain', ondrain);

source.removeListener('end', onend);
source.removeListener('close', onend);
source.removeListener('close', onclose);

dest.removeListener('pause', onpause);
dest.removeListener('resume', onresume);
Expand Down
5 changes: 0 additions & 5 deletions lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -608,12 +608,7 @@ SecurePair.prototype._destroy = function() {
self.cleartext.writable = self.cleartext.readable = false;

process.nextTick(function() {
self.encrypted.emit('end');
if (self.encrypted.onend) self.encrypted.onend();
self.encrypted.emit('close');

self.cleartext.emit('end');
if (self.cleartext.onend) self.cleartext.onend();
self.cleartext.emit('close');
});
}
Expand Down
2 changes: 1 addition & 1 deletion test/simple/test-https-eof-for-eom.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ server.listen(common.PORT, function() {
bodyBuffer += s;
});

res.on('end', function() {
res.on('close', function() {
console.log('5) Client got "end" event.');
gotEnd = true;
});
Expand Down
4 changes: 4 additions & 0 deletions test/simple/test-stream-pipe-cleanup.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ Writable.prototype.end = function () {
this.endCalls++;
}

Writable.prototype.destroy = function () {
this.endCalls++;
}

function Readable () {
this.readable = true;
stream.Stream.call(this);
Expand Down

2 comments on commit 75a0cf9

@dukejeffrie
Copy link

Choose a reason for hiding this comment

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

This change seems to have broken restler and other https clients, since the 'end' event is no longer reliably fired. The change to test-https-eof-for-eom corroborates this hypothesis. Shouldn't the close handler fire 'end' if it's not been fired yet?

@ry
Copy link
Author

@ry ry commented on 75a0cf9 May 14, 2011

Choose a reason for hiding this comment

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

Damn - that's not good. No, the "end" event is not guaranteed to happen, but since this is the stable branch Node needs to match what people are doing...

Please sign in to comment.