From 79f7f349993343d40d6e014e141753cd3c70b145 Mon Sep 17 00:00:00 2001 From: Mark Plomer Date: Wed, 8 Apr 2015 15:00:00 +0200 Subject: [PATCH] http: fix missing close event on aborted response When sending a response after client aborted the connection in the same tick when the socket is already destroyed but the socket-close event is still not delivered, you did not get either a response-finish or a response-close event. This fix of the race-condition needs an additional flag "_finishOrCloseEmitted" in OutgoingMessage. --- lib/_http_outgoing.js | 3 + lib/_http_server.js | 12 ++- .../test-http-response-close-event-race.js | 88 +++++++++++++++++++ 3 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-response-close-event-race.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 31022617558f44..33180db5ae6e11 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -62,6 +62,7 @@ function OutgoingMessage() { this._trailer = ''; this.finished = false; + this._finishOrCloseEmitted = false; this._hangupClose = false; this._headerSent = false; @@ -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'); } diff --git a/lib/_http_server.js b/lib/_http_server.js index 99903024c26d14..b557067adf595c 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -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) { @@ -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; diff --git a/test/parallel/test-http-response-close-event-race.js b/test/parallel/test-http-response-close-event-race.js new file mode 100644 index 00000000000000..0c7760e8098baf --- /dev/null +++ b/test/parallel/test-http-response-close-event-race.js @@ -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'); + + 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(); + } + }); + }); +}