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 lib/_http_outgoing.js #1143

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

var self = this;

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

var err = new Error('stream closed prematurely');

var outputCallbacks = self.outputCallbacks;

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

for (var i = 0; i < outputCallbacks.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you cache the variable and reset it rather than using it raw in the for loop and resetting it afterwards?

Copy link
Author

Choose a reason for hiding this comment

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

I worry the callback function over which we have no control might do something fishy and mess up our state while we're working with it. If such things are nothing we are supposed to be guarding against, then I'll change it of course.

var callback = outputCallbacks[i];
if (typeof callback === 'function')
Copy link
Contributor

Choose a reason for hiding this comment

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

is this check necessary? will a callback ever not be of type 'function'?

Copy link
Author

Choose a reason for hiding this comment

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

It could be null or undefined if the user didn't specify one...

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe just:

if (callback) 
  callback(err);

callback(err);
}
});
}
util.inherits(OutgoingMessage, Stream);

Expand Down Expand Up @@ -159,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 @@ -407,9 +424,10 @@ Object.defineProperty(OutgoingMessage.prototype, 'headersSent', {

OutgoingMessage.prototype.write = function(chunk, encoding, callback) {
var self = this;
var err;

if (this.finished) {
var err = new Error('write after end');
err = new Error('write after end');
Copy link
Member

Choose a reason for hiding this comment

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

It would be slightly more memory-efficient here and elsewhere to create the Error object inside the nextTick callback.

Copy link
Author

Choose a reason for hiding this comment

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

The error object is created outside because otherwise it wouldn't contain the correct stack trace.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's a fair point. What's the motivation for emitting the exception as an error instead of throwing it? Both are going to be disruptive.

As a small aside, I notice that OutgoingMessage#write() still returns true, indicating it's okay to call it again. That seems like a buglet to me.

Copy link
Author

Choose a reason for hiding this comment

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

I left all return values unchanged. Currently return values are pretty much only used as an indicator of whether
the write had to be buffered (false) or was written instantly (true).

The reason for not throwing at that point is that the user still has no reliable (documented!) way of checking the state of the stream beforehand. He would have to wrap every write in try catch...

That's another thing bugging me about the state of the current stream API.

But even having to check some field every time before I do a write... nope! I'd rather handle all errors at one point: in my callback.

process.nextTick(function() {
self.emit('error', err);
if (callback) callback(err);
Expand All @@ -423,19 +441,25 @@ OutgoingMessage.prototype.write = function(chunk, encoding, callback) {
}

if (!this._hasBody) {
debug('This type of response MUST NOT have a body. ' +
'Ignoring write() calls.');
err = new Error('this type of response must not have a body');
Copy link
Contributor

Choose a reason for hiding this comment

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

"this type of response" is rather vague, could we change it to anything that is more concise?

Copy link
Author

Choose a reason for hiding this comment

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

Sure

process.nextTick(function() {
self.emit('error', err);
if (callback) callback(err);
});
return true;
}

if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
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 @@ -511,11 +535,24 @@ 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) {
//If the user tried to write data, tell him it failed
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: please put a space after the slashes and use punctuation. I've been told some people take offense at gendered pronouns in comments.

I'm not sure how I feel about turning write-after-end into an error. It's arguably the proper thing to do but I worry that it's going to break a lot of existing code.

var err = new Error('write after end');
process.nextTick(function() {
self.emit('error', err);
if (callback) callback(err);
});
} else {
//If he only wanted to end the stream anyways, lucky for him
if (callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Above here, you check for typeof callback === 'function' rather than a truthy check. Could we stick to one?

Strictly speaking, I don't think there's a perf difference between them.

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 @@ -39,7 +39,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;
Copy link
Member

Choose a reason for hiding this comment

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

Style error: space after if.


gotError = true;
switch (er.code) {
// This is the expected case
Expand Down
23 changes: 21 additions & 2 deletions test/parallel/test-http-many-ended-pipelines.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,28 @@ var numRequests = 20;
var done = 0;

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

var callbackCalled = false;

res.end('ok', function(){
Copy link
Member

Choose a reason for hiding this comment

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

Style error: space before {.

assert.ok(!callbackCalled);
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the stylistic changes here and below? Try to keep the diff as noise-free as possible.

callbackCalled = true;
});

// We might get a socket already closed error here
// Not really a surpise
res.on('error', function(){});

res.on('close', function(){
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Author

Choose a reason for hiding this comment

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

Apparently "make jslint" doesn't check tests. Curious. I wonder why.

process.nextTick(function(){
assert.ok(callbackCalled, "end() callback should've been called");
Copy link
Member

Choose a reason for hiding this comment

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

Femto-nit: it's customary to use single quotes for strings.

});
});

// Oh no! The connection died!
req.socket.destroy();
if (++done == numRequests)

Copy link
Member

Choose a reason for hiding this comment

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

Can you undo the whitespace changes here and below?

if (++done === numRequests)
server.close();
});

Expand All @@ -31,5 +48,7 @@ for (var i = 0; i < numRequests; i++) {
'Host: some.host.name\r\n'+
'\r\n\r\n');
}

client.end();

client.pipe(process.stdout);