Skip to content

Commit

Permalink
http: fix double AbortSignal registration
Browse files Browse the repository at this point in the history
Fix an issue where AbortSignals are registered twice

PR-URL: #37730
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
Linkgoron authored and ruyadorno committed Mar 24, 2021
1 parent 9f61cbd commit e9c161c
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 17 deletions.
17 changes: 12 additions & 5 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -321,16 +327,17 @@ function ClientRequest(input, options, cb) {
});

try {
const newSocket = options.createConnection(options, oncreate);
const newSocket = optsWithoutSignal.createConnection(optsWithoutSignal,
oncreate);
if (newSocket) {
oncreate(null, newSocket);
}
} catch (err) {
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));
}
}
}
Expand Down
73 changes: 73 additions & 0 deletions test/parallel/test-http-agent-abort-controller.js
Original file line number Diff line number Diff line change
@@ -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();
}));
52 changes: 49 additions & 3 deletions test/parallel/test-http-client-abort-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../common');
const http = require('http');
const assert = require('assert');
const { getEventListeners } = require('events');

{
// abort
Expand Down Expand Up @@ -71,23 +72,68 @@ 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();
assert.strictEqual(req.aborted, false);
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);
}));
}
15 changes: 7 additions & 8 deletions test/parallel/test-http2-client-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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 });
Expand All @@ -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();

Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-https-abortcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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');
Expand Down

0 comments on commit e9c161c

Please sign in to comment.