From c784f15588bf659d0301fea1ba3587748c03f0bb Mon Sep 17 00:00:00 2001 From: Daniele Belardi Date: Sun, 27 Dec 2020 20:59:59 +0100 Subject: [PATCH] Revert "http: use `autoDestroy: true` in incoming message" This reverts commits: * 55e83cbe957de2c752521557ffd74d4496fa8b81 * 6120028ee3ca88d009c62711a225b1af7fad1919 * 70eaf55a9d2da25742df79a862edf16f122fdfbb * 5ae96908696b7a017df3efb48ec1f8c8be5c24b8 * f20a88fb866917d7dd489b88c233a5e123ff3271 * a6bf74eac00516fd0767b585c8f304857dddd2aa * 8154e47e2b407a906f2c1270e5fc76fe466cc886 Because of the breaking change in the order of emitting the `close` event in `IncomingMessage` described in: https://github.com/nodejs/node/pull/33035#issuecomment-751476367 PR-URL: https://github.com/nodejs/node/pull/36647 Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina Reviewed-By: Rich Trott --- lib/_http_client.js | 19 +++++++- lib/_http_incoming.js | 46 +++++-------------- lib/_http_server.js | 14 +++++- ...est-http-client-incomingmessage-destroy.js | 25 ---------- ...est-http-server-incomingmessage-destroy.js | 25 ---------- 5 files changed, 43 insertions(+), 86 deletions(-) delete mode 100644 test/parallel/test-http-client-incomingmessage-destroy.js delete mode 100644 test/parallel/test-http-server-incomingmessage-destroy.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 782b8e9714fe81..6fb5dd65cb368c 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -430,13 +430,25 @@ function socketCloseListener() { req.destroyed = true; if (res) { // Socket closed before we emitted 'end' below. + // TOOD(ronag): res.destroy(err) if (!res.complete) { - res.destroy(connResetException('aborted')); + res.aborted = true; + res.emit('aborted'); + if (res.listenerCount('error') > 0) { + res.emit('error', connResetException('aborted')); + } } req._closed = true; 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 { if (!req.socket._hadError) { @@ -685,6 +697,7 @@ function responseKeepAlive(req) { req.destroyed = true; if (req.res) { + req.res.destroyed = true; // Detach socket from IncomingMessage to avoid destroying the freed // socket in IncomingMessage.destroy(). req.res.socket = null; @@ -739,6 +752,10 @@ function requestOnPrefinish() { function emitFreeNT(req) { req._closed = true; req.emit('close'); + if (req.res) { + req.res.emit('close'); + } + if (req.socket) { req.socket.emit('free'); } diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js index 22ab591b7ad78c..7943c69f54d911 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -27,7 +27,7 @@ const { Symbol } = primordials; -const { Readable, finished } = require('stream'); +const Stream = require('stream'); const kHeaders = Symbol('kHeaders'); const kHeadersCount = Symbol('kHeadersCount'); @@ -54,7 +54,7 @@ function IncomingMessage(socket) { }; } - Readable.call(this, streamOptions); + Stream.Readable.call(this, { autoDestroy: false, ...streamOptions }); this._readableState.readingMore = true; @@ -89,8 +89,8 @@ function IncomingMessage(socket) { // read by the user, so there's no point continuing to handle it. this._dumped = false; } -ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype); -ObjectSetPrototypeOf(IncomingMessage, Readable); +ObjectSetPrototypeOf(IncomingMessage.prototype, Stream.Readable.prototype); +ObjectSetPrototypeOf(IncomingMessage, Stream.Readable); ObjectDefineProperty(IncomingMessage.prototype, 'connection', { get: function() { @@ -160,31 +160,19 @@ IncomingMessage.prototype._read = function _read(n) { readStart(this.socket); }; + // It's possible that the socket will be destroyed, and removed from // 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(err, cb) { - if (!this.readableEnded || !this.complete) { - this.aborted = true; - this.emit('aborted'); - } - - // If aborted and the underlying socket is not already destroyed, - // destroy it. - // We have to check if the socket is already destroyed because finished - // does not call the callback when this methdod is invoked from `_http_client` - // in `test/parallel/test-http-client-spurious-aborted.js` - if (this.socket && !this.socket.destroyed && this.aborted) { - this.socket.destroy(err); - const cleanup = finished(this.socket, (e) => { - cleanup(); - onError(this, e || err, cb); - }); - } else { - onError(this, err, cb); - } +IncomingMessage.prototype.destroy = function destroy(error) { + // TODO(ronag): Implement in terms of _destroy + this.destroyed = true; + if (this.socket) + this.socket.destroy(error); + return this; }; + IncomingMessage.prototype._addHeaderLines = _addHeaderLines; function _addHeaderLines(headers, n) { if (headers && headers.length) { @@ -361,16 +349,6 @@ IncomingMessage.prototype._dump = function _dump() { } }; -function onError(self, error, cb) { - // This is to keep backward compatible behavior. - // An error is emitted only if there are listeners attached to the event. - if (self.listenerCount('error') === 0) { - cb(); - } else { - cb(error); - } -} - module.exports = { IncomingMessage, readStart, diff --git a/lib/_http_server.js b/lib/_http_server.js index 8cd10bb3a00194..419b08a7a0986d 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -575,7 +575,14 @@ function socketOnClose(socket, state) { function abortIncoming(incoming) { while (incoming.length) { const req = incoming.shift(); - req.destroy(connResetException('aborted')); + // TODO(ronag): req.destroy(err) + req.aborted = true; + req.destroyed = true; + req.emit('aborted'); + if (req.listenerCount('error') > 0) { + req.emit('error', connResetException('aborted')); + } + req.emit('close'); } // Abort socket._httpMessage ? } @@ -734,9 +741,14 @@ 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'); } } diff --git a/test/parallel/test-http-client-incomingmessage-destroy.js b/test/parallel/test-http-client-incomingmessage-destroy.js deleted file mode 100644 index a0823d37786365..00000000000000 --- a/test/parallel/test-http-client-incomingmessage-destroy.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; - -const common = require('../common'); -const { createServer, get } = require('http'); -const assert = require('assert'); - -const server = createServer(common.mustCall((req, res) => { - res.writeHead(200); - res.write('Part of res.'); -})); - -function onUncaught(error) { - assert.strictEqual(error.message, 'Destroy test'); - server.close(); -} - -process.on('uncaughtException', common.mustCall(onUncaught)); - -server.listen(0, () => { - get({ - port: server.address().port - }, common.mustCall((res) => { - res.destroy(new Error('Destroy test')); - })); -}); diff --git a/test/parallel/test-http-server-incomingmessage-destroy.js b/test/parallel/test-http-server-incomingmessage-destroy.js deleted file mode 100644 index cfe7e4feecba45..00000000000000 --- a/test/parallel/test-http-server-incomingmessage-destroy.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict'; - -const common = require('../common'); -const { createServer, get } = require('http'); -const assert = require('assert'); - -const server = createServer(common.mustCall((req, res) => { - req.destroy(new Error('Destroy test')); -})); - -function onUncaught(error) {} - -process.on('uncaughtException', common.mustNotCall(onUncaught)); - -server.listen(0, common.mustCall(() => { - get({ - port: server.address().port - }, (res) => { - res.resume(); - }).on('error', (error) => { - assert.strictEqual(error.message, 'socket hang up'); - assert.strictEqual(error.code, 'ECONNRESET'); - server.close(); - }); -}));