Skip to content
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.

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

Always call callbacks in outgoing streams #2240

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 40 additions & 8 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,24 @@ function OutgoingMessage() {
this._header = null;
this._headers = null;
this._headerNames = {};

// Call remaining callbacks if stream closes prematurely
this.once('close', function() {
if (this.output.length === 0) return;

const err = new Error('stream closed prematurely');
const outputCallbacks = this.outputCallbacks;

this.output = [];
this.outputEncodings = [];
this.outputCallbacks = [];

for (var i = 0; i < outputCallbacks.length; i++) {
let callback = outputCallbacks[i];
if (callback)
callback(err);
}
});
}
util.inherits(OutgoingMessage, Stream);

Expand Down Expand Up @@ -162,12 +180,8 @@ OutgoingMessage.prototype._writeRaw = function(data, encoding, callback) {

// Directly write to socket.
return connection.write(data, encoding, callback);
} else if (connection && connection.destroyed) {
// The socket was destroyed. If we're still trying to write to it,
// then we haven't gotten the 'close' event yet.
return false;
} else {
// buffer, as long as we're not destroyed.
// buffer, as long as we didn't get the "close" event
return this._buffer(data, encoding, callback);
}
};
Expand Down Expand Up @@ -430,10 +444,13 @@ OutgoingMessage.prototype.write = function(chunk, encoding, callback) {
throw new TypeError('first argument must be a string or Buffer');
}


// If we get an empty string or buffer, then just do nothing, and
// signal the user to keep writing.
if (chunk.length === 0) return true;
if (chunk.length === 0) {
if (typeof callback === 'function')
process.nextTick(callback);
return true;
}

var len, ret;
if (this.chunkedEncoding) {
Expand Down Expand Up @@ -517,11 +534,26 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {
throw new TypeError('first argument must be a string or Buffer');
}

var self = this;

if (this.finished) {
if (data && data.length > 0) {
// Report that writing the data has failed
// because the stream was already 'ended'.
var err = new Error('write after end');
process.nextTick(function() {
self.emit('error', err);
if (callback) callback(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is a strange hybrid between err-back and event emitter.

/cc @nodejs/tsc Feedback on basically emitting the error twice?

Copy link
Contributor

Choose a reason for hiding this comment

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

One or the other seems better. Maybe if (typeof callback === 'function') callback(err); else self.emit('error', err);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this had been discussed some time back. See nodejs/node-v0.x-archive#7477 where this change originated from.

IMHO it should only be one or the other, like @cjihrig says. If a callback is supplied, then pass the error and not emit. Otherwise emit.

Copy link
Member

Choose a reason for hiding this comment

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

+1. Down with double errors!!

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that solution. @laino mind making that change?

});
} else {
// The user wanted to end the stream anyway,
// so we don't need to report a failure.
if (callback)
process.nextTick(callback);
}
return false;
}

var self = this;
function finish() {
self.emit('finish');
}
Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-http-destroyed-socket-write2.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ server.listen(common.PORT, function() {
var sawEnd = false;

req.on('error', function(er) {
assert(!gotError);

// Each failed write will cause an error, but
// we are only interested in one
if (gotError) return;

gotError = true;
switch (er.code) {
// This is the expected case
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-http-many-ended-pipelines.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@ var numRequests = 20;
var done = 0;

var server = http.createServer(function(req, res) {
res.end('ok');

res.end('ok', common.mustCall(function() {}));

// We *might* get a socket already closed error here, which
// occurs when the socket was destroyed before we finished
// writing our data.
res.on('error', function() {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Then lets check the error here, just to be sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

But should that error cause the test to fail? I don't believe this particular fail means what the test was created to test has failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

As it is we simply ignore everything right? As the comment says, if we check if the error is actually socket closed error, wouldn't it be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see what you're saying. good point. yes, checking for expected errors, but still throwing on unexpected errors, would be a smart thing to do.


// Oh no! The connection died!
req.socket.destroy();
Expand Down