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: fix missing close event on aborted response #1373

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
3 changes: 3 additions & 0 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ function OutgoingMessage() {
this._trailer = '';

this.finished = false;
this._finishOrCloseEmitted = false;
this._hangupClose = false;
this._headerSent = false;

Expand Down Expand Up @@ -517,6 +518,8 @@ OutgoingMessage.prototype.end = function(data, encoding, callback) {

var self = this;
function finish() {
if (self._finishOrCloseEmitted) return;
self._finishOrCloseEmitted = true;
self.emit('finish');
}

Expand Down
12 changes: 11 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ function onServerResponseClose() {
// Ergo, we need to deal with stale 'close' events and handle the case
// where the ServerResponse object has already been deconstructed.
// Fortunately, that requires only a single if check. :-)
if (this._httpMessage) this._httpMessage.emit('close');
if (this._httpMessage && !this._httpMessage._finishOrCloseEmitted) {
this._httpMessage._finishOrCloseEmitted = true;
this._httpMessage.emit('close');
}
}

ServerResponse.prototype.assignSocket = function(socket) {
Expand All @@ -137,6 +140,13 @@ ServerResponse.prototype.assignSocket = function(socket) {

ServerResponse.prototype.detachSocket = function(socket) {
assert(socket._httpMessage === this);
if (socket.destroyed && !socket._httpMessage._finishOrCloseEmitted) {
var httpMessage = socket._httpMessage;
httpMessage._finishOrCloseEmitted = true;
setImmediate(function() {
httpMessage.emit('close');
});
}
socket.removeListener('close', onServerResponseClose);
socket._httpMessage = null;
this.socket = this.connection = null;
Expand Down
88 changes: 88 additions & 0 deletions test/parallel/test-http-response-close-event-race.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
var common = require('../common');
var assert = require('assert');
var http = require('http');

var clientRequest = null;
var serverResponseFinishedOrClosed = 0;
var testTickCount = 3;

var server = http.createServer(function (req, res) {
console.log('server: request');
Copy link
Contributor

Choose a reason for hiding this comment

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

usually, passing tests should be silent; could you remove the expected console.logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I could remove them - I kept with the other tests which also contain console.logs (output is hidden anyway in normal test run).
I thought, especially race conditions are much more understandable with some debugging output - also for future refactoring/debugging.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep them for now :) I've a bit delayed this, but going to review it next week!


res.on('finish', function () {
console.log('server: response finish');
serverResponseFinishedOrClosed++;
});
res.on('close', function () {
console.log('server: response close');
serverResponseFinishedOrClosed++;
});

console.log('client: aborting request');
clientRequest.abort();

var ticks = 0;
function tick() {
console.log('server: tick ' + ticks + (req.connection.destroyed ? ' (connection destroyed!)' : ''));

if (ticks < testTickCount) {
ticks++;
setImmediate(tick);
} else {
sendResponse();
}
}
tick();

function sendResponse() {
console.log('server: sending response');
res.writeHead(200, {'Content-Type': 'text/plain'});
res.end('Response\n');
console.log('server: res.end() returned');

handleResponseEnd();
}
});

server.on('listening', function () {
console.log('server: listening on port ' + common.PORT);
console.log('-----------------------------------------------------');
startRequest();
});

server.on('connection', function (connection) {
console.log('server: connection');
connection.on('close', function () { console.log('server: connection close'); });
});

server.on('close', function () {
console.log('server: close');
});

server.listen(common.PORT);

function startRequest() {
console.log('client: starting request - testing with ' + testTickCount + ' ticks after abort()');
serverResponseFinishedOrClosed = 0;

var options = {port: common.PORT, path: '/'};
clientRequest = http.get(options, function () {});
clientRequest.on('error', function () {});
}

function handleResponseEnd() {
setImmediate(function () {
setImmediate(function () {
assert.equal(serverResponseFinishedOrClosed, 1, 'Expected either one "finish" or one "close" event on the response for aborted connections (got ' + serverResponseFinishedOrClosed + ')');
console.log('server: ended request with correct finish/close event');
console.log('-----------------------------------------------------');

if (testTickCount > 0) {
testTickCount--;
startRequest();
} else {
server.close();
}
});
});
}