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

HTTP outgoing queue bug fix (#2639) #3068

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 26 additions & 6 deletions lib/_http_outgoing.js
Expand Up @@ -51,7 +51,8 @@ function OutgoingMessage() {
this.outputCallbacks = [];

this.writable = true;

this.explicitfinish = false;

this._last = false;
this.chunkedEncoding = false;
this.shouldKeepAlive = true;
Expand Down Expand Up @@ -133,8 +134,12 @@ OutgoingMessage.prototype._writeRaw = function(data, encoding, callback) {
encoding = null;
}

//if this happens and callback is finish - it will be fired in bad time!!!
assert(!(this.connection && data.length === 0 && this.output.length > 0));

if (data.length === 0) {
if (typeof callback === 'function')
//callback cannot be called (finish) when the connection is not assigned
if (typeof callback === 'function' && this.connection)
Copy link
Contributor

Choose a reason for hiding this comment

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

connection is already asserted above.

Copy link
Member

Choose a reason for hiding this comment

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

We have a better fix, feel free to skip this PR.

Choose a reason for hiding this comment

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

We have a better fix, feel free to skip this PR.

Are you going to open another PR?

Copy link
Member

Choose a reason for hiding this comment

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

@arthurschreiber of course!

process.nextTick(callback);
return true;
}
Expand Down Expand Up @@ -536,7 +541,9 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {

var self = this;
function finish() {
self.emit('finish');
//finish MUST NOT be called when connection is not assigned
assert(self.connection);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this situation ever happen?

self.emit('finish');
}

if (typeof callback === 'function')
Expand Down Expand Up @@ -574,6 +581,12 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {
} else {
// Force a flush, HACK.
ret = this._send('', 'binary', finish);
if (!this.connection) {
//if there is nothing to send and connection is not assigned, nothing is put into output buffer, so no callback will be called after write
//and nexttick callback in _writeraw MUST be skipped - so finish must be explicitly called after flushing this message as it becomes the first in queue
//this should be really rewritten
this.explicitfinish = true;
}
}

if (this.connection && data)
Expand All @@ -587,14 +600,14 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {
if (this.output.length === 0 &&
this.connection &&
this.connection._httpMessage === this) {
this._finish();
this.prefinish();
}

return ret;
};


OutgoingMessage.prototype._finish = function() {
OutgoingMessage.prototype.prefinish = function() {
assert(this.connection);
this.emit('prefinish');
};
Expand Down Expand Up @@ -642,7 +655,14 @@ OutgoingMessage.prototype._flush = function() {

if (this.finished) {
// This is a queue to the server or client to bring in the next this.
this._finish();
this.prefinish();
if (this.explicitfinish) {
//finish MUST NOT be called when connection is not assigned
//message was already finished and finish callback was not in the queue
assert(this.connection);
this.emit('finish');
}

} else if (ret) {
// This is necessary to prevent https from breaking
this.emit('drain');
Expand Down