From 28e6626ce7020b490438e3ee8a8188a59c5f856f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 28 Apr 2020 19:17:57 +0200 Subject: [PATCH] http: set IncomingMessage.destroyed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IncomingMessage is a Readable stream and should properly set the destroyed property. PR-URL: https://github.com/nodejs/node/pull/33131 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: Juan José Arboleda --- lib/_http_client.js | 4 ++ lib/_http_incoming.js | 2 + lib/_http_server.js | 9 +++- .../test-http-client-res-destroyed.js | 41 +++++++++++++++++++ test/parallel/test-http-connect-req-res.js | 5 ++- test/parallel/test-http-connect.js | 5 ++- .../test-http-pause-resume-one-end.js | 9 +++- test/parallel/test-http-req-res-close.js | 12 ++++++ test/parallel/test-http-response-close.js | 8 ++++ test/parallel/test-http-server-stale-close.js | 3 ++ 10 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-client-res-destroyed.js diff --git a/lib/_http_client.js b/lib/_http_client.js index c78289138e756a..22b695e6064809 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -422,10 +422,12 @@ function socketCloseListener() { req.emit('close'); if (!res.aborted && res.readable) { res.on('end', function() { + this.destroyed = true; this.emit('close'); }); res.push(null); } else { + res.destroyed = true; res.emit('close'); } } else { @@ -539,6 +541,7 @@ function socketOnData(d) { socket.readableFlowing = null; req.emit(eventName, res, socket, bodyHead); + req.destroyed = true; req.emit('close'); } else { // Requested Upgrade or used CONNECT method, but have no handler. @@ -710,6 +713,7 @@ function requestOnPrefinish() { function emitFreeNT(req) { req.emit('close'); if (req.res) { + req.res.destroyed = true; req.res.emit('close'); } diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js index f309e32e36dc7b..60081279a05e42 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -119,6 +119,8 @@ IncomingMessage.prototype._read = function _read(n) { // any messages, before ever calling this. In that case, just skip // it, since something else is destroying this connection anyway. IncomingMessage.prototype.destroy = function destroy(error) { + // TODO(ronag): Implement in terms of _destroy + this.destroyed = true; if (this.socket) this.socket.destroy(error); }; diff --git a/lib/_http_server.js b/lib/_http_server.js index c8b27dcd42cb5b..9dfeef746636c5 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -205,7 +205,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.destroyed = true; + this._httpMessage.emit('close'); + } } ServerResponse.prototype.assignSocket = function assignSocket(socket) { @@ -534,6 +537,7 @@ function abortIncoming(incoming) { while (incoming.length) { const req = incoming.shift(); req.aborted = true; + req.destroyed = true; req.emit('aborted'); req.emit('close'); } @@ -660,11 +664,13 @@ function clearIncoming(req) { if (parser && parser.incoming === req) { if (req.readableEnded) { parser.incoming = null; + req.destroyed = true; req.emit('close'); } else { req.on('end', clearIncoming); } } else { + req.destroyed = true; req.emit('close'); } } @@ -708,6 +714,7 @@ function resOnFinish(req, res, socket, state, server) { } function emitCloseNT(self) { + self.destroyed = true; self.emit('close'); } diff --git a/test/parallel/test-http-client-res-destroyed.js b/test/parallel/test-http-client-res-destroyed.js new file mode 100644 index 00000000000000..188ab06c155731 --- /dev/null +++ b/test/parallel/test-http-client-res-destroyed.js @@ -0,0 +1,41 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +{ + const server = http.createServer(common.mustCall((req, res) => { + res.end('asd'); + })); + + server.listen(0, common.mustCall(() => { + http.get({ + port: server.address().port + }, common.mustCall((res) => { + assert.strictEqual(res.destroyed, false); + res.destroy(); + assert.strictEqual(res.destroyed, true); + res.on('close', common.mustCall(() => { + server.close(); + })); + })); + })); +} + +{ + const server = http.createServer(common.mustCall((req, res) => { + res.end('asd'); + })); + + server.listen(0, common.mustCall(() => { + http.get({ + port: server.address().port + }, common.mustCall((res) => { + assert.strictEqual(res.destroyed, false); + res.on('close', common.mustCall(() => { + assert.strictEqual(res.destroyed, true); + server.close(); + })).resume(); + })); + })); +} diff --git a/test/parallel/test-http-connect-req-res.js b/test/parallel/test-http-connect-req-res.js index 893c621852be89..cd6e55e0898c66 100644 --- a/test/parallel/test-http-connect-req-res.js +++ b/test/parallel/test-http-connect-req-res.js @@ -33,7 +33,10 @@ server.listen(0, common.mustCall(function() { path: 'example.com:443' }, common.mustNotCall()); - req.on('close', common.mustCall()); + assert.strictEqual(req.destroyed, false); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); + })); req.on('connect', common.mustCall(function(res, socket, firstBodyChunk) { console.error('Client got CONNECT request'); diff --git a/test/parallel/test-http-connect.js b/test/parallel/test-http-connect.js index be853de347e0cb..47fd7316d0cf1d 100644 --- a/test/parallel/test-http-connect.js +++ b/test/parallel/test-http-connect.js @@ -62,7 +62,10 @@ server.listen(0, common.mustCall(() => { assert.strictEqual(socket._httpMessage, req); })); - req.on('close', common.mustCall()); + assert.strictEqual(req.destroyed, false); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); + })); req.on('connect', common.mustCall((res, socket, firstBodyChunk) => { // Make sure this request got removed from the pool. diff --git a/test/parallel/test-http-pause-resume-one-end.js b/test/parallel/test-http-pause-resume-one-end.js index 4886a7d490e413..34bf3be478f09c 100644 --- a/test/parallel/test-http-pause-resume-one-end.js +++ b/test/parallel/test-http-pause-resume-one-end.js @@ -22,6 +22,7 @@ 'use strict'; const common = require('../common'); const http = require('http'); +const assert = require('assert'); const server = http.Server(function(req, res) { res.writeHead(200, { 'Content-Type': 'text/plain' }); @@ -43,6 +44,12 @@ server.listen(0, common.mustCall(function() { }); })); - res.on('end', common.mustCall()); + res.on('end', common.mustCall(() => { + assert.strictEqual(res.destroyed, false); + })); + assert.strictEqual(res.destroyed, false); + res.on('close', common.mustCall(() => { + assert.strictEqual(res.destroyed, true); + })); })); })); diff --git a/test/parallel/test-http-req-res-close.js b/test/parallel/test-http-req-res-close.js index daba55f43427c2..329515ccd76ffb 100644 --- a/test/parallel/test-http-req-res-close.js +++ b/test/parallel/test-http-req-res-close.js @@ -8,13 +8,25 @@ const server = http.Server(common.mustCall((req, res) => { let resClosed = false; res.end(); + let resFinished = false; res.on('finish', common.mustCall(() => { + resFinished = true; + assert.strictEqual(resClosed, false); + assert.strictEqual(res.destroyed, false); assert.strictEqual(resClosed, false); })); + assert.strictEqual(req.destroyed, false); res.on('close', common.mustCall(() => { resClosed = true; + assert.strictEqual(resFinished, true); + assert.strictEqual(res.destroyed, true); + })); + assert.strictEqual(req.destroyed, false); + req.on('end', common.mustCall(() => { + assert.strictEqual(req.destroyed, false); })); req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); assert.strictEqual(req._readableState.ended, true); })); res.socket.on('close', () => server.close()); diff --git a/test/parallel/test-http-response-close.js b/test/parallel/test-http-response-close.js index 4c36003357980b..0435cd0ee37da5 100644 --- a/test/parallel/test-http-response-close.js +++ b/test/parallel/test-http-response-close.js @@ -22,6 +22,7 @@ 'use strict'; const common = require('../common'); const http = require('http'); +const assert = require('assert'); { const server = http.createServer( @@ -39,7 +40,9 @@ const http = require('http'); res.on('data', common.mustCall(() => { res.destroy(); })); + assert.strictEqual(res.destroyed, false); res.on('close', common.mustCall(() => { + assert.strictEqual(res.destroyed, true); server.close(); })); }) @@ -61,7 +64,12 @@ const http = require('http'); http.get( { port: server.address().port }, common.mustCall((res) => { + assert.strictEqual(res.destroyed, false); + res.on('end', common.mustCall(() => { + assert.strictEqual(res.destroyed, false); + })); res.on('close', common.mustCall(() => { + assert.strictEqual(res.destroyed, true); server.close(); })); res.resume(); diff --git a/test/parallel/test-http-server-stale-close.js b/test/parallel/test-http-server-stale-close.js index d67c4d31c36e37..b9322ed9fcf83a 100644 --- a/test/parallel/test-http-server-stale-close.js +++ b/test/parallel/test-http-server-stale-close.js @@ -23,6 +23,7 @@ require('../common'); const http = require('http'); const fork = require('child_process').fork; +const assert = require('assert'); if (process.env.NODE_TEST_FORK_PORT) { const req = http.request({ @@ -37,7 +38,9 @@ if (process.env.NODE_TEST_FORK_PORT) { const server = http.createServer((req, res) => { res.writeHead(200, { 'Content-Length': '42' }); req.pipe(res); + assert.strictEqual(req.destroyed, false); req.on('close', () => { + assert.strictEqual(req.destroyed, true); server.close(); res.end(); });