-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Changes from all commits
2dd0bc2
d733196
8f20ed6
6d4bd06
e712b26
10d5da8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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++) { | ||
var callback = outputCallbacks[i]; | ||
if (typeof callback === 'function') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this check necessary? will a callback ever not be of type There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
|
@@ -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); | ||
} | ||
}; | ||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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); | ||
|
@@ -432,10 +450,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) { | ||
|
@@ -511,11 +532,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Above here, you check for 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'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,27 @@ var numRequests = 20; | |
var done = 0; | ||
|
||
var server = http.createServer(function(req, res) { | ||
res.end('ok'); | ||
|
||
var callbackCalled = false; | ||
|
||
res.end('ok', function() { | ||
assert.ok(!callbackCalled); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kind of worries me here - wouldn't emitting an error break the test? Why are we just ignoring it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Failing to write some data will now always create an error. Which is a change that would've broken the test. Previously there would've been no error even though the data was never written. |
||
|
||
res.on('close', function() { | ||
process.nextTick(function() { | ||
assert.ok(callbackCalled, "end() callback should've been called"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
}); | ||
|
@@ -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); |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.