From 8ff38ec989e6c7adeb17c8ecbf8d0bd1232fae21 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 28 Apr 2020 13:57:32 +0200 Subject: [PATCH 1/2] http: don't destroy completed request Calling destroy() on a completed ClientRequest, i.e. once 'close' will be emitted should be a noop. Also before emitting 'close' destroyed === true. Fixes: https://github.com/nodejs/node/issues/32851 --- lib/_http_client.js | 4 +++ ...est-http-client-agent-abort-close-event.js | 2 ++ .../test-http-client-agent-end-close-event.js | 2 ++ test/parallel/test-http-client-close-event.js | 1 + .../test-http-client-override-global-agent.js | 6 ++-- test/parallel/test-http-client-set-timeout.js | 1 + .../test-http-client-timeout-event.js | 6 +++- .../test-http-client-timeout-option.js | 5 ++- test/parallel/test-http-client-timeout.js | 1 + test/parallel/test-http-keepalive-free.js | 36 +++++++++++++++++++ 10 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-keepalive-free.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 22b695e6064809..2c8023a70c728a 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -413,6 +413,8 @@ function socketCloseListener() { // the `socketOnData`. const parser = socket.parser; const res = req.res; + + req.destroyed = true; if (res) { // Socket closed before we emitted 'end' below. if (!res.complete) { @@ -667,7 +669,9 @@ function responseKeepAlive(req) { // handlers have a chance to run. defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, req); + req.destroyed = true; if (req.res) { + // TODO(ronag): res.destroyed=true? // Detach socket from IncomingMessage to avoid destroying the freed // socket in IncomingMessage.destroy(). req.res.socket = null; diff --git a/test/parallel/test-http-client-agent-abort-close-event.js b/test/parallel/test-http-client-agent-abort-close-event.js index 437d2aad222780..69a48da232c369 100644 --- a/test/parallel/test-http-client-agent-abort-close-event.js +++ b/test/parallel/test-http-client-agent-abort-close-event.js @@ -1,5 +1,6 @@ 'use strict'; const common = require('../common'); +const assert = require('assert'); const http = require('http'); const server = http.createServer(common.mustNotCall()); @@ -16,6 +17,7 @@ server.listen(0, common.mustCall(() => { .on('socket', common.mustNotCall()) .on('response', common.mustNotCall()) .on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); server.close(); keepAliveAgent.destroy(); })) diff --git a/test/parallel/test-http-client-agent-end-close-event.js b/test/parallel/test-http-client-agent-end-close-event.js index de3ce193fbba9d..ce202f4d2f283c 100644 --- a/test/parallel/test-http-client-agent-end-close-event.js +++ b/test/parallel/test-http-client-agent-end-close-event.js @@ -1,5 +1,6 @@ 'use strict'; const common = require('../common'); +const assert = require('assert'); const http = require('http'); const server = http.createServer(common.mustCall((req, res) => { @@ -18,6 +19,7 @@ server.listen(0, common.mustCall(() => { .on('response', common.mustCall((res) => { res .on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); server.close(); keepAliveAgent.destroy(); })) diff --git a/test/parallel/test-http-client-close-event.js b/test/parallel/test-http-client-close-event.js index b539423a80f8b5..7097a247859f5e 100644 --- a/test/parallel/test-http-client-close-event.js +++ b/test/parallel/test-http-client-close-event.js @@ -22,6 +22,7 @@ server.listen(0, common.mustCall(() => { })); req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); assert.strictEqual(errorEmitted, true); server.close(); })); diff --git a/test/parallel/test-http-client-override-global-agent.js b/test/parallel/test-http-client-override-global-agent.js index f046abaa74e45d..0efaded5e419b0 100644 --- a/test/parallel/test-http-client-override-global-agent.js +++ b/test/parallel/test-http-client-override-global-agent.js @@ -21,6 +21,8 @@ function makeRequest() { const req = http.get({ port: server.address().port }); - req.on('close', () => - server.close()); + req.on('close', () => { + assert.strictEqual(req.destroyed, true); + server.close(); + }); } diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index 51b6622a6b71cc..8cb83e3f1869d0 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -38,6 +38,7 @@ server.listen(0, mustCall(() => { })); req.on('close', mustCall(() => { + strictEqual(req.destroyed, true); server.close(); })); diff --git a/test/parallel/test-http-client-timeout-event.js b/test/parallel/test-http-client-timeout-event.js index 71767bef8f1641..d5a63622bad03b 100644 --- a/test/parallel/test-http-client-timeout-event.js +++ b/test/parallel/test-http-client-timeout-event.js @@ -21,6 +21,7 @@ 'use strict'; const common = require('../common'); +const assert = require('assert'); const http = require('http'); const options = { @@ -38,7 +39,10 @@ server.listen(0, options.host, function() { req.on('error', function() { // This space is intentionally left blank }); - req.on('close', common.mustCall(() => server.close())); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); + server.close(); + })); req.setTimeout(1); req.on('timeout', common.mustCall(() => { diff --git a/test/parallel/test-http-client-timeout-option.js b/test/parallel/test-http-client-timeout-option.js index c21ec88e855208..1003c28b5def56 100644 --- a/test/parallel/test-http-client-timeout-option.js +++ b/test/parallel/test-http-client-timeout-option.js @@ -23,7 +23,10 @@ server.listen(0, options.host, function() { req.on('error', function() { // This space is intentionally left blank }); - req.on('close', common.mustCall(() => server.close())); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.destroyed, true); + server.close(); + })); let timeout_events = 0; req.on('timeout', common.mustCall(() => timeout_events += 1)); diff --git a/test/parallel/test-http-client-timeout.js b/test/parallel/test-http-client-timeout.js index 90a3bebebc9e13..b83bd1ddf010fe 100644 --- a/test/parallel/test-http-client-timeout.js +++ b/test/parallel/test-http-client-timeout.js @@ -41,6 +41,7 @@ server.listen(0, options.host, function() { // This space intentionally left blank }); req.on('close', function() { + assert.strictEqual(req.destroyed, true); server.close(); }); function destroy() { diff --git a/test/parallel/test-http-keepalive-free.js b/test/parallel/test-http-keepalive-free.js new file mode 100644 index 00000000000000..0252c284962b39 --- /dev/null +++ b/test/parallel/test-http-keepalive-free.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +const http = require('http'); + +for (const method of ['abort', 'destroy']) { + const server = http.createServer(common.mustCall((req, res) => { + res.end(req.url); + })); + server.listen(0, common.mustCall(() => { + const agent = http.Agent({ keepAlive: true }); + + const req = http + .request({ + port: server.address().port, + agent + }) + .on('socket', common.mustCall((socket) => { + socket.on('free', common.mustCall()); + })) + .on('response', common.mustCall((res) => { + assert.strictEqual(req.destroyed, false); + res.on('end', () => { + assert.strictEqual(req.destroyed, true); + req[method](); + assert.strictEqual(req.socket.destroyed, false); + agent.destroy(); + server.close(); + }).resume(); + })) + .end(); + assert.strictEqual(req.destroyed, false); + })); +} From b4b030a992aff8558aa2fbb7aaf0d66343285f1b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 9 May 2020 11:04:23 +0200 Subject: [PATCH 2/2] fixup --- lib/_http_client.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 2c8023a70c728a..23e82c807fc6e0 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -671,7 +671,7 @@ function responseKeepAlive(req) { req.destroyed = true; if (req.res) { - // TODO(ronag): res.destroyed=true? + req.res.destroyed = true; // Detach socket from IncomingMessage to avoid destroying the freed // socket in IncomingMessage.destroy(). req.res.socket = null; @@ -717,7 +717,6 @@ function requestOnPrefinish() { function emitFreeNT(req) { req.emit('close'); if (req.res) { - req.res.destroyed = true; req.res.emit('close'); }