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.

Sign up for GitHub

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_server: correct order prefinish=>finish #3059

Closed
wants to merge 2 commits into from
Closed

http_server: correct order prefinish=>finish #3059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2daec6
http_server: correct order `prefinish`=>`finish`
indutny Sep 25, 2015
23b911d
fix
indutny Sep 25, 2015
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
11 changes: 10 additions & 1 deletion lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ function OutgoingMessage() {
this._header = null;
this._headers = null;
this._headerNames = {};
this._prefinish = false;
}
util.inherits(OutgoingMessage, Stream);

Expand Down Expand Up @@ -526,7 +527,14 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {

var self = this;
function finish() {
self.emit('finish');
// NOTE: It is very important to wait for `prefinish` event.
// It is emitted only when the response becomes active in the pipeline.
// The ordering of requests/responses in `_http_server.js` is dependent
// on this, doing it in different order will result in crash.
if (self._prefinish)
self.emit('finish');
else
self.on('prefinish', finish);
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 if this is possible then there's something else wrong with the logic. From the documentation:

After [the finish] event, no more events will be emitted on the response object.

So how could this function ever be called more than once?

Copy link

Choose a reason for hiding this comment

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

I don't think this changes that logic. This change ensures that the prefinish event is actually emited before the finish event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I'm trying to point out that it should be impossible to call this function twice. So the added check shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not about calling twice. It is about the order of events. The check handles the case where finish is emitted before the prefinish was emitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is finish(). I find I confusing that it could be called in the preFinish case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed very confusing because we don't use streams API for these events. We should move to them in the future. Feel free to open issue for this, @trevnorris

}

if (typeof callback === 'function')
Expand Down Expand Up @@ -586,6 +594,7 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {

OutgoingMessage.prototype._finish = function() {
assert(this.connection);
this._prefinish = true;
this.emit('prefinish');
};

Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-http-pipeline-regr-2639.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const net = require('net');
const http = require('http');

var count = 0;
const server = http.createServer(function(req, res) {
const id = count++;
setTimeout(function() {
res.end('hello');
}, (2 - id) * 10);

if (id === 1)
server.close();
}).listen(common.PORT, function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

common.mustCall()?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 I think this is way too much :) If this is not going to work - we are pretty much in a bad situation. What about verifying the count instead?

Also, there is a possibly better fix for this: #3068 .

const s = net.connect(common.PORT);

const req = 'GET / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n';
const big = req + req;
s.end(big);
s.resume();
});