From e9c161ce129fa75e618fb74cfa3493591728d80d Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 12 Mar 2021 20:24:00 +0200 Subject: [PATCH] http: fix double AbortSignal registration Fix an issue where AbortSignals are registered twice PR-URL: https://github.com/nodejs/node/pull/37730 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Rich Trott --- lib/_http_client.js | 17 +++-- .../test-http-agent-abort-controller.js | 73 +++++++++++++++++++ .../test-http-client-abort-destroy.js | 52 ++++++++++++- test/parallel/test-http2-client-destroy.js | 15 ++-- test/parallel/test-https-abortcontroller.js | 3 +- 5 files changed, 143 insertions(+), 17 deletions(-) create mode 100644 test/parallel/test-http-agent-abort-controller.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 70c8a26f3fc17f..57a36183405170 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -304,14 +304,20 @@ function ClientRequest(input, options, cb) { options.headers); } + let optsWithoutSignal = options; + if (optsWithoutSignal.signal) { + optsWithoutSignal = ObjectAssign({}, options); + delete optsWithoutSignal.signal; + } + // initiate connection if (this.agent) { - this.agent.addRequest(this, options); + this.agent.addRequest(this, optsWithoutSignal); } else { // No agent, default to Connection:close. this._last = true; this.shouldKeepAlive = false; - if (typeof options.createConnection === 'function') { + if (typeof optsWithoutSignal.createConnection === 'function') { const oncreate = once((err, socket) => { if (err) { process.nextTick(() => this.emit('error', err)); @@ -321,7 +327,8 @@ function ClientRequest(input, options, cb) { }); try { - const newSocket = options.createConnection(options, oncreate); + const newSocket = optsWithoutSignal.createConnection(optsWithoutSignal, + oncreate); if (newSocket) { oncreate(null, newSocket); } @@ -329,8 +336,8 @@ function ClientRequest(input, options, cb) { oncreate(err); } } else { - debug('CLIENT use net.createConnection', options); - this.onSocket(net.createConnection(options)); + debug('CLIENT use net.createConnection', optsWithoutSignal); + this.onSocket(net.createConnection(optsWithoutSignal)); } } } diff --git a/test/parallel/test-http-agent-abort-controller.js b/test/parallel/test-http-agent-abort-controller.js new file mode 100644 index 00000000000000..c5ece3ab353bf0 --- /dev/null +++ b/test/parallel/test-http-agent-abort-controller.js @@ -0,0 +1,73 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); +const Agent = http.Agent; +const { getEventListeners, once } = require('events'); +const agent = new Agent(); +const server = http.createServer(); + +server.listen(0, common.mustCall(async () => { + const port = server.address().port; + const host = 'localhost'; + const options = { + port: port, + host: host, + _agentKey: agent.getName({ port, host }) + }; + + async function postCreateConnection() { + const ac = new AbortController(); + const { signal } = ac; + const connection = agent.createConnection({ ...options, signal }); + assert.strictEqual(getEventListeners(signal, 'abort').length, 1); + ac.abort(); + const [err] = await once(connection, 'error'); + assert.strictEqual(err?.name, 'AbortError'); + } + + async function preCreateConnection() { + const ac = new AbortController(); + const { signal } = ac; + ac.abort(); + const connection = agent.createConnection({ ...options, signal }); + const [err] = await once(connection, 'error'); + assert.strictEqual(err?.name, 'AbortError'); + } + + async function agentAsParam() { + const ac = new AbortController(); + const { signal } = ac; + const request = http.get({ + port: server.address().port, + path: '/hello', + agent: agent, + signal, + }); + assert.strictEqual(getEventListeners(signal, 'abort').length, 1); + ac.abort(); + const [err] = await once(request, 'error'); + assert.strictEqual(err?.name, 'AbortError'); + } + + async function agentAsParamPreAbort() { + const ac = new AbortController(); + const { signal } = ac; + ac.abort(); + const request = http.get({ + port: server.address().port, + path: '/hello', + agent: agent, + signal, + }); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); + const [err] = await once(request, 'error'); + assert.strictEqual(err?.name, 'AbortError'); + } + + await postCreateConnection(); + await preCreateConnection(); + await agentAsParam(); + await agentAsParamPreAbort(); + server.close(); +})); diff --git a/test/parallel/test-http-client-abort-destroy.js b/test/parallel/test-http-client-abort-destroy.js index 7af63a1ef408c0..11b1390a839b68 100644 --- a/test/parallel/test-http-client-abort-destroy.js +++ b/test/parallel/test-http-client-abort-destroy.js @@ -2,6 +2,7 @@ const common = require('../common'); const http = require('http'); const assert = require('assert'); +const { getEventListeners } = require('events'); { // abort @@ -71,19 +72,20 @@ const assert = require('assert'); { - // Destroy with AbortSignal + // Destroy post-abort sync with AbortSignal const server = http.createServer(common.mustNotCall()); const controller = new AbortController(); - + const { signal } = controller; server.listen(0, common.mustCall(() => { - const options = { port: server.address().port, signal: controller.signal }; + const options = { port: server.address().port, signal }; const req = http.get(options, common.mustNotCall()); req.on('error', common.mustCall((err) => { assert.strictEqual(err.code, 'ABORT_ERR'); assert.strictEqual(err.name, 'AbortError'); server.close(); })); + assert.strictEqual(getEventListeners(signal, 'abort').length, 1); assert.strictEqual(req.aborted, false); assert.strictEqual(req.destroyed, false); controller.abort(); @@ -91,3 +93,47 @@ const assert = require('assert'); assert.strictEqual(req.destroyed, true); })); } + +{ + // Use post-abort async AbortSignal + const server = http.createServer(common.mustNotCall()); + const controller = new AbortController(); + const { signal } = controller; + server.listen(0, common.mustCall(() => { + const options = { port: server.address().port, signal }; + const req = http.get(options, common.mustNotCall()); + req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ABORT_ERR'); + assert.strictEqual(err.name, 'AbortError'); + })); + + req.on('close', common.mustCall(() => { + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, true); + server.close(); + })); + + assert.strictEqual(getEventListeners(signal, 'abort').length, 1); + process.nextTick(() => controller.abort()); + })); +} + +{ + // Use pre-aborted AbortSignal + const server = http.createServer(common.mustNotCall()); + const controller = new AbortController(); + const { signal } = controller; + server.listen(0, common.mustCall(() => { + controller.abort(); + const options = { port: server.address().port, signal }; + const req = http.get(options, common.mustNotCall()); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); + req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ABORT_ERR'); + assert.strictEqual(err.name, 'AbortError'); + server.close(); + })); + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.destroyed, true); + })); +} diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index f257cf6f0006fa..52b0744cd2190d 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -8,9 +8,8 @@ if (!common.hasCrypto) const assert = require('assert'); const h2 = require('http2'); const { kSocket } = require('internal/http2/util'); -const { kEvents } = require('internal/event_target'); const Countdown = require('../common/countdown'); - +const { getEventListeners } = require('events'); { const server = h2.createServer(); server.listen(0, common.mustCall(() => { @@ -180,11 +179,11 @@ const Countdown = require('../common/countdown'); client.on('close', common.mustCall()); const { signal } = controller; - assert.strictEqual(signal[kEvents].get('abort'), undefined); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); client.on('error', common.mustCall(() => { // After underlying stream dies, signal listener detached - assert.strictEqual(signal[kEvents].get('abort'), undefined); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); })); const req = client.request({}, { signal }); @@ -198,7 +197,7 @@ const Countdown = require('../common/countdown'); assert.strictEqual(req.aborted, false); assert.strictEqual(req.destroyed, false); // Signal listener attached - assert.strictEqual(signal[kEvents].get('abort').size, 1); + assert.strictEqual(getEventListeners(signal, 'abort').length, 1); controller.abort(); @@ -219,16 +218,16 @@ const Countdown = require('../common/countdown'); const { signal } = controller; controller.abort(); - assert.strictEqual(signal[kEvents].get('abort'), undefined); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); client.on('error', common.mustCall(() => { // After underlying stream dies, signal listener detached - assert.strictEqual(signal[kEvents].get('abort'), undefined); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); })); const req = client.request({}, { signal }); // Signal already aborted, so no event listener attached. - assert.strictEqual(signal[kEvents].get('abort'), undefined); + assert.strictEqual(getEventListeners(signal, 'abort').length, 0); assert.strictEqual(req.aborted, false); // Destroyed on same tick as request made diff --git a/test/parallel/test-https-abortcontroller.js b/test/parallel/test-https-abortcontroller.js index 80821285acf95e..19aa9a6fca2bdf 100644 --- a/test/parallel/test-https-abortcontroller.js +++ b/test/parallel/test-https-abortcontroller.js @@ -7,7 +7,7 @@ if (!common.hasCrypto) const fixtures = require('../common/fixtures'); const https = require('https'); const assert = require('assert'); -const { once } = require('events'); +const { once, getEventListeners } = require('events'); const options = { key: fixtures.readKey('agent1-key.pem'), @@ -29,6 +29,7 @@ const options = { rejectUnauthorized: false, signal: ac.signal, }); + assert.strictEqual(getEventListeners(ac.signal, 'abort').length, 1); process.nextTick(() => ac.abort()); const [ err ] = await once(req, 'error'); assert.strictEqual(err.name, 'AbortError');