From 1b275e09b59d3b95fcab74d1461f955900bba3b8 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 12:39:50 +0100 Subject: [PATCH 1/9] http: align OutgoingMessage and ClientRequest destroy Added .destroyed property to OutgoingMessage and ClientRequest to align with streams. Fixed ClientRequest.destroy to dump res and re-use socket in agent pool aligning it with abort. --- lib/_http_client.js | 62 ++++++++++++------- lib/_http_outgoing.js | 6 ++ ...ient-abort-keep-alive-queued-tcp-socket.js | 57 ++++++++++------- test/parallel/test-http-client-close-event.js | 4 +- test/parallel/test-http-outgoing-proto.js | 7 +++ 5 files changed, 88 insertions(+), 48 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index fef4b635a00166..676ed3b2c50aaa 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -29,6 +29,7 @@ const { ObjectAssign, ObjectKeys, ObjectSetPrototypeOf, + Symbol } = primordials; const net = require('net'); @@ -65,6 +66,7 @@ const { } = require('internal/dtrace'); const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; +const kOnSocket = Symbol('kOnSocket'); function validateHost(host, name) { if (host !== null && host !== undefined && typeof host !== 'string') { @@ -337,25 +339,47 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader() { }; ClientRequest.prototype.abort = function abort() { - if (!this.aborted) { - process.nextTick(emitAbortNT, this); + if (this.aborted) { + return; } this.aborted = true; + process.nextTick(emitAbortNT, this); + this.destroy(); +}; + +ClientRequest.prototype.destroy = function destroy(err) { + if (this.destroyed) { + return; + } + this.destroyed = true; // If we're aborting, we don't care about any more response data. if (this.res) { this.res._dump(); } - // In the event that we don't have a socket, we will pop out of - // the request queue through handling in onSocket. - if (this.socket) { - // in-progress - this.socket.destroy(); + if (!this.socket) { + this.once(kOnSocket, (socket) => { + if (this.agent) { + socket.emit('free'); + } else { + socket.destroy(); + } + + if (!this.aborted && !err) { + err = connResetException('socket hang up'); + } + + if (err) { + this.emit('error', err); + } + this.emit('close'); + }); + } else { + this.socket.destroy(err); } }; - function emitAbortNT(req) { req.emit('abort'); } @@ -686,6 +710,12 @@ function emitFreeNT(req) { } function tickOnSocket(req, socket) { + req.emit(kOnSocket, socket); + + if (req.destroyed) { + return; + } + const parser = parsers.alloc(); req.socket = socket; parser.initialize(HTTPParser.RESPONSE, @@ -746,23 +776,9 @@ function listenSocketTimeout(req) { } ClientRequest.prototype.onSocket = function onSocket(socket) { - process.nextTick(onSocketNT, this, socket); + process.nextTick(tickOnSocket, this, socket); }; -function onSocketNT(req, socket) { - if (req.aborted) { - // If we were aborted while waiting for a socket, skip the whole thing. - if (!req.agent) { - socket.destroy(); - } else { - req.emit('close'); - socket.emit('free'); - } - } else { - tickOnSocket(req, socket); - } -} - ClientRequest.prototype._deferToConnect = _deferToConnect; function _deferToConnect(method, arguments_, cb) { // This function is for calls that need to happen once the socket is diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index b02edc7f34fa8e..a7bd6af60073a7 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -93,6 +93,7 @@ function OutgoingMessage() { this.outputSize = 0; this.writable = true; + this.destroyed = false; this._last = false; this.chunkedEncoding = false; @@ -277,6 +278,11 @@ OutgoingMessage.prototype.setTimeout = function setTimeout(msecs, callback) { // any messages, before ever calling this. In that case, just skip // it, since something else is destroying this connection anyway. OutgoingMessage.prototype.destroy = function destroy(error) { + if (this.destroyed) { + return; + } + this.destroyed = true; + if (this.socket) { this.socket.destroy(error); } else { diff --git a/test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js b/test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js index 6282aa3da7caf2..c9614f01c3db3f 100644 --- a/test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js +++ b/test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js @@ -3,34 +3,45 @@ const common = require('../common'); const assert = require('assert'); const http = require('http'); -let socketsCreated = 0; -class Agent extends http.Agent { - createConnection(options, oncreate) { - const socket = super.createConnection(options, oncreate); - socketsCreated++; - return socket; +for (const destroyer of ['destroy', 'abort']) { + let socketsCreated = 0; + + class Agent extends http.Agent { + createConnection(options, oncreate) { + const socket = super.createConnection(options, oncreate); + socketsCreated++; + return socket; + } } -} -const server = http.createServer((req, res) => res.end()); + const server = http.createServer((req, res) => res.end()); -server.listen(0, common.mustCall(() => { - const port = server.address().port; - const agent = new Agent({ - keepAlive: true, - maxSockets: 1 - }); + server.listen(0, common.mustCall(() => { + const port = server.address().port; + const agent = new Agent({ + keepAlive: true, + maxSockets: 1 + }); - http.get({ agent, port }, (res) => res.resume()); + http.get({ agent, port }, (res) => res.resume()); - const req = http.get({ agent, port }, common.mustNotCall()); - req.abort(); + const req = http.get({ agent, port }, common.mustNotCall()); + req[destroyer](); - http.get({ agent, port }, common.mustCall((res) => { - res.resume(); - assert.strictEqual(socketsCreated, 1); - agent.destroy(); - server.close(); + if (destroyer === 'destroy') { + req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ECONNRESET'); + })); + } else { + req.on('error', common.mustNotCall()); + } + + http.get({ agent, port }, common.mustCall((res) => { + res.resume(); + assert.strictEqual(socketsCreated, 1); + agent.destroy(); + server.close(); + })); })); -})); +} diff --git a/test/parallel/test-http-client-close-event.js b/test/parallel/test-http-client-close-event.js index 7573931ac48ef6..b539423a80f8b5 100644 --- a/test/parallel/test-http-client-close-event.js +++ b/test/parallel/test-http-client-close-event.js @@ -14,12 +14,12 @@ server.listen(0, common.mustCall(() => { const req = http.get({ port: server.address().port }, common.mustNotCall()); let errorEmitted = false; - req.on('error', (err) => { + req.on('error', common.mustCall((err) => { errorEmitted = true; assert.strictEqual(err.constructor, Error); assert.strictEqual(err.message, 'socket hang up'); assert.strictEqual(err.code, 'ECONNRESET'); - }); + })); req.on('close', common.mustCall(() => { assert.strictEqual(errorEmitted, true); diff --git a/test/parallel/test-http-outgoing-proto.js b/test/parallel/test-http-outgoing-proto.js index 3c62eadc003ff3..4a07d18c601c5c 100644 --- a/test/parallel/test-http-outgoing-proto.js +++ b/test/parallel/test-http-outgoing-proto.js @@ -122,3 +122,10 @@ assert.throws(() => { name: 'TypeError', message: 'Invalid character in trailer content ["404"]' }); + +{ + const outgoingMessage = new OutgoingMessage(); + assert.strictEqual(outgoingMessage.destroyed, false); + outgoingMessage.destroy(); + assert.strictEqual(outgoingMessage.destroyed, true); +} From 54de4897871d24001e01aaa79758327b1e7e8f76 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 13:04:52 +0100 Subject: [PATCH 2/9] fixup: use destroy when available --- lib/_http_client.js | 4 +- lib/internal/streams/destroy.js | 3 +- .../test-http-client-abort-destroy.js | 71 +++++++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-http-client-abort-destroy.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 676ed3b2c50aaa..9d10a64e4dc76d 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -449,7 +449,9 @@ function socketErrorListener(err) { const req = socket._httpMessage; debug('SOCKET ERROR:', err.message, err.stack); - if (req) { + // If writableFinished then the error came from the readable/response + // side and will be emitted there. + if (req && !req.writableFinished) { // For Safety. Some additional errors might fire later on // and we need to make sure we don't double-fire the error event. req.socket._hadError = true; diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 3a953afd445649..3c6bdd01c21f88 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -163,10 +163,9 @@ function isRequest(stream) { // Normalize destroy for legacy. function destroyer(stream, err) { - // request.destroy just do .end - .abort is what we want + if (typeof stream.destroy === 'function') return stream.destroy(err); if (isRequest(stream)) return stream.abort(); if (isRequest(stream.req)) return stream.req.abort(); - if (typeof stream.destroy === 'function') return stream.destroy(err); if (typeof stream.close === 'function') return stream.close(); } diff --git a/test/parallel/test-http-client-abort-destroy.js b/test/parallel/test-http-client-abort-destroy.js new file mode 100644 index 00000000000000..442d9d72eba2bd --- /dev/null +++ b/test/parallel/test-http-client-abort-destroy.js @@ -0,0 +1,71 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +{ + // abort + + const server = http.createServer(common.mustCall((req, res) => { + res.end('Hello'); + })); + + server.listen(0, common.mustCall(() => { + const options = { port: server.address().port }; + const req = http.get(options, common.mustCall((res) => { + res.on('data', (data) => { + req.abort(); + assert.strictEqual(req.aborted, true); + assert.strictEqual(req.destroyed, true); + server.close(); + }); + })); + req.on('error', common.mustNotCall()); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, false); + })); +} + +{ + // destroy + res + + const server = http.createServer(common.mustCall((req, res) => { + res.end('Hello'); + })); + + server.listen(0, common.mustCall(() => { + const options = { port: server.address().port }; + const req = http.get(options, common.mustCall((res) => { + res.on('data', (data) => { + req.destroy(); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, true); + server.close(); + }); + })); + req.on('error', common.mustNotCall()); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, false); + })); +} + + +{ + // destroy + + const server = http.createServer(common.mustNotCall((req, res) => { + })); + + server.listen(0, common.mustCall(() => { + const options = { port: server.address().port }; + const req = http.get(options, common.mustNotCall()); + req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ECONNRESET'); + })); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, false); + req.destroy(); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, true); + })); +} From 88ce62458ad86f55dcc276940c5632151b60f486 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 15:00:46 +0100 Subject: [PATCH 3/9] fixup: reduce changes --- lib/internal/streams/destroy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 3c6bdd01c21f88..6eb46c7f7c454e 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -163,9 +163,9 @@ function isRequest(stream) { // Normalize destroy for legacy. function destroyer(stream, err) { - if (typeof stream.destroy === 'function') return stream.destroy(err); if (isRequest(stream)) return stream.abort(); if (isRequest(stream.req)) return stream.req.abort(); + if (typeof stream.destroy === 'function') return stream.destroy(err); if (typeof stream.close === 'function') return stream.close(); } From 2746f705dfe17003a18aa6b18dc764a7c392145e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 15:12:54 +0100 Subject: [PATCH 4/9] fixup --- lib/_http_client.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 9d10a64e4dc76d..676ed3b2c50aaa 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -449,9 +449,7 @@ function socketErrorListener(err) { const req = socket._httpMessage; debug('SOCKET ERROR:', err.message, err.stack); - // If writableFinished then the error came from the readable/response - // side and will be emitted there. - if (req && !req.writableFinished) { + if (req) { // For Safety. Some additional errors might fire later on // and we need to make sure we don't double-fire the error event. req.socket._hadError = true; From 0998e9b150d335667978c55f29242373ff93e88a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 15:19:53 +0100 Subject: [PATCH 5/9] fixup: make more similar to original --- lib/_http_client.js | 56 ++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 676ed3b2c50aaa..183233e494c108 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -66,7 +66,7 @@ const { } = require('internal/dtrace'); const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; -const kOnSocket = Symbol('kOnSocket'); +const kError = Symbol('kError'); function validateHost(host, name) { if (host !== null && host !== undefined && typeof host !== 'string') { @@ -358,28 +358,17 @@ ClientRequest.prototype.destroy = function destroy(err) { this.res._dump(); } - if (!this.socket) { - this.once(kOnSocket, (socket) => { - if (this.agent) { - socket.emit('free'); - } else { - socket.destroy(); - } - - if (!this.aborted && !err) { - err = connResetException('socket hang up'); - } - - if (err) { - this.emit('error', err); - } - this.emit('close'); - }); - } else { + // In the event that we don't have a socket, we will pop out of + // the request queue through handling in onSocket. + if (this.socket) { + // in-progress this.socket.destroy(err); + } else if (err) { + this[kError] = err; } }; + function emitAbortNT(req) { req.emit('abort'); } @@ -710,12 +699,6 @@ function emitFreeNT(req) { } function tickOnSocket(req, socket) { - req.emit(kOnSocket, socket); - - if (req.destroyed) { - return; - } - const parser = parsers.alloc(); req.socket = socket; parser.initialize(HTTPParser.RESPONSE, @@ -776,9 +759,30 @@ function listenSocketTimeout(req) { } ClientRequest.prototype.onSocket = function onSocket(socket) { - process.nextTick(tickOnSocket, this, socket); + process.nextTick(onSocketNT, this, socket); }; +function onSocketNT(req, socket) { + if (req.destroyed) { + // If we were aborted while waiting for a socket, skip the whole thing. + if (!req.agent) { + socket.destroy(req[kError]); + } else { + socket.emit('free'); + let err = req[kError]; + if (!req.aborted && !err) { + err = connResetException('socket hang up'); + } + if (err) { + req.emit('error', err); + } + req.emit('close'); + } + } else { + tickOnSocket(req, socket); + } +} + ClientRequest.prototype._deferToConnect = _deferToConnect; function _deferToConnect(method, arguments_, cb) { // This function is for calls that need to happen once the socket is From 0245e4e142bde5fbb596b05cae19a51a75fdc1a3 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 16:55:30 +0100 Subject: [PATCH 6/9] fixup: refactor for TODO --- lib/_http_client.js | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 183233e494c108..c447b695c8b45b 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -361,13 +361,29 @@ ClientRequest.prototype.destroy = function destroy(err) { // In the event that we don't have a socket, we will pop out of // the request queue through handling in onSocket. if (this.socket) { - // in-progress - this.socket.destroy(err); + _destroy(this, this.socket, err); } else if (err) { this[kError] = err; } }; +function _destroy(req, socket, err) { + // TODO (ronag): Check if socket was used at all (e.g. headersSent) and + // re-use it in that case. `req.socket` just checks whether the socket was + // assigned to the request and *might* have been used. + if (!req.agent || req.socket) { + socket.destroy(err); + } else { + socket.emit('free'); + if (!req.aborted && !err) { + err = connResetException('socket hang up'); + } + if (err) { + req.emit('error', err); + } + req.emit('close'); + } +} function emitAbortNT(req) { req.emit('abort'); @@ -764,20 +780,7 @@ ClientRequest.prototype.onSocket = function onSocket(socket) { function onSocketNT(req, socket) { if (req.destroyed) { - // If we were aborted while waiting for a socket, skip the whole thing. - if (!req.agent) { - socket.destroy(req[kError]); - } else { - socket.emit('free'); - let err = req[kError]; - if (!req.aborted && !err) { - err = connResetException('socket hang up'); - } - if (err) { - req.emit('error', err); - } - req.emit('close'); - } + _destroy(req, socket, req[kError]); } else { tickOnSocket(req, socket); } From 6f11b2431985f0de6555f731d746cbc3adc5608d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 17:57:46 +0100 Subject: [PATCH 7/9] fixup: reuse if not headersSent --- lib/_http_client.js | 5 +---- lib/_http_outgoing.js | 4 ++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index c447b695c8b45b..b3802d6baf37eb 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -368,10 +368,7 @@ ClientRequest.prototype.destroy = function destroy(err) { }; function _destroy(req, socket, err) { - // TODO (ronag): Check if socket was used at all (e.g. headersSent) and - // re-use it in that case. `req.socket` just checks whether the socket was - // assigned to the request and *might* have been used. - if (!req.agent || req.socket) { + if (!req.agent || req.headersSent) { socket.destroy(err); } else { socket.emit('free'); diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index a7bd6af60073a7..7d28ebb49dc995 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -335,6 +335,10 @@ function _writeRaw(data, encoding, callback) { return false; } + if (this.destroyed) { + return false; + } + if (typeof encoding === 'function') { callback = encoding; encoding = null; From 2750006c0eff27e9164b450c2efdaa4a41f4ec47 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 19:00:37 +0100 Subject: [PATCH 8/9] Revert "fixup: reuse if not headersSent" This reverts commit 6f11b2431985f0de6555f731d746cbc3adc5608d. --- lib/_http_client.js | 5 ++++- lib/_http_outgoing.js | 4 ---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index b3802d6baf37eb..c447b695c8b45b 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -368,7 +368,10 @@ ClientRequest.prototype.destroy = function destroy(err) { }; function _destroy(req, socket, err) { - if (!req.agent || req.headersSent) { + // TODO (ronag): Check if socket was used at all (e.g. headersSent) and + // re-use it in that case. `req.socket` just checks whether the socket was + // assigned to the request and *might* have been used. + if (!req.agent || req.socket) { socket.destroy(err); } else { socket.emit('free'); diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 7d28ebb49dc995..a7bd6af60073a7 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -335,10 +335,6 @@ function _writeRaw(data, encoding, callback) { return false; } - if (this.destroyed) { - return false; - } - if (typeof encoding === 'function') { callback = encoding; encoding = null; From f24b4a7199e74802402f0799e62d990052d41182 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 8 Mar 2020 23:12:15 +0100 Subject: [PATCH 9/9] fixup: test --- test/parallel/test-http-client-abort-destroy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-client-abort-destroy.js b/test/parallel/test-http-client-abort-destroy.js index 442d9d72eba2bd..6db2ea5682e922 100644 --- a/test/parallel/test-http-client-abort-destroy.js +++ b/test/parallel/test-http-client-abort-destroy.js @@ -49,7 +49,6 @@ const assert = require('assert'); })); } - { // destroy @@ -61,6 +60,7 @@ const assert = require('assert'); const req = http.get(options, common.mustNotCall()); req.on('error', common.mustCall((err) => { assert.strictEqual(err.code, 'ECONNRESET'); + server.close(); })); assert.strictEqual(req.aborted, false); assert.strictEqual(req.destroyed, false);