Skip to content

Commit e9c161c

Browse files
Linkgoronruyadorno
Linkgoron
authored and
ruyadorno
committed
http: fix double AbortSignal registration
Fix an issue where AbortSignals are registered twice PR-URL: #37730 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>
1 parent 9f61cbd commit e9c161c

File tree

5 files changed

+143
-17
lines changed

5 files changed

+143
-17
lines changed

‎lib/_http_client.js‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -304,14 +304,20 @@ function ClientRequest(input, options, cb) {
304304
options.headers);
305305
}
306306

307+
let optsWithoutSignal = options;
308+
if (optsWithoutSignal.signal) {
309+
optsWithoutSignal = ObjectAssign({}, options);
310+
delete optsWithoutSignal.signal;
311+
}
312+
307313
// initiate connection
308314
if (this.agent) {
309-
this.agent.addRequest(this, options);
315+
this.agent.addRequest(this, optsWithoutSignal);
310316
} else {
311317
// No agent, default to Connection:close.
312318
this._last = true;
313319
this.shouldKeepAlive = false;
314-
if (typeof options.createConnection === 'function') {
320+
if (typeof optsWithoutSignal.createConnection === 'function') {
315321
const oncreate = once((err, socket) => {
316322
if (err) {
317323
process.nextTick(() => this.emit('error', err));
@@ -321,16 +327,17 @@ function ClientRequest(input, options, cb) {
321327
});
322328

323329
try {
324-
const newSocket = options.createConnection(options, oncreate);
330+
const newSocket = optsWithoutSignal.createConnection(optsWithoutSignal,
331+
oncreate);
325332
if (newSocket) {
326333
oncreate(null, newSocket);
327334
}
328335
} catch (err) {
329336
oncreate(err);
330337
}
331338
} else {
332-
debug('CLIENT use net.createConnection', options);
333-
this.onSocket(net.createConnection(options));
339+
debug('CLIENT use net.createConnection', optsWithoutSignal);
340+
this.onSocket(net.createConnection(optsWithoutSignal));
334341
}
335342
}
336343
}

‎test/parallel/test-http-agent-abort-controller.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const http = require('http');
5+
const Agent = http.Agent;
6+
const { getEventListeners, once } = require('events');
7+
const agent = new Agent();
8+
const server = http.createServer();
9+
10+
server.listen(0, common.mustCall(async () => {
11+
const port = server.address().port;
12+
const host = 'localhost';
13+
const options = {
14+
port: port,
15+
host: host,
16+
_agentKey: agent.getName({ port, host })
17+
};
18+
19+
async function postCreateConnection() {
20+
const ac = new AbortController();
21+
const { signal } = ac;
22+
const connection = agent.createConnection({ ...options, signal });
23+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
24+
ac.abort();
25+
const [err] = await once(connection, 'error');
26+
assert.strictEqual(err?.name, 'AbortError');
27+
}
28+
29+
async function preCreateConnection() {
30+
const ac = new AbortController();
31+
const { signal } = ac;
32+
ac.abort();
33+
const connection = agent.createConnection({ ...options, signal });
34+
const [err] = await once(connection, 'error');
35+
assert.strictEqual(err?.name, 'AbortError');
36+
}
37+
38+
async function agentAsParam() {
39+
const ac = new AbortController();
40+
const { signal } = ac;
41+
const request = http.get({
42+
port: server.address().port,
43+
path: '/hello',
44+
agent: agent,
45+
signal,
46+
});
47+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
48+
ac.abort();
49+
const [err] = await once(request, 'error');
50+
assert.strictEqual(err?.name, 'AbortError');
51+
}
52+
53+
async function agentAsParamPreAbort() {
54+
const ac = new AbortController();
55+
const { signal } = ac;
56+
ac.abort();
57+
const request = http.get({
58+
port: server.address().port,
59+
path: '/hello',
60+
agent: agent,
61+
signal,
62+
});
63+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
64+
const [err] = await once(request, 'error');
65+
assert.strictEqual(err?.name, 'AbortError');
66+
}
67+
68+
await postCreateConnection();
69+
await preCreateConnection();
70+
await agentAsParam();
71+
await agentAsParamPreAbort();
72+
server.close();
73+
}));

‎test/parallel/test-http-client-abort-destroy.js‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
const common = require('../common');
33
const http = require('http');
44
const assert = require('assert');
5+
const { getEventListeners } = require('events');
56

67
{
78
// abort
@@ -71,23 +72,68 @@ const assert = require('assert');
7172

7273

7374
{
74-
// Destroy with AbortSignal
75+
// Destroy post-abort sync with AbortSignal
7576

7677
const server = http.createServer(common.mustNotCall());
7778
const controller = new AbortController();
78-
79+
const { signal } = controller;
7980
server.listen(0, common.mustCall(() => {
80-
const options = { port: server.address().port, signal: controller.signal };
81+
const options = { port: server.address().port, signal };
8182
const req = http.get(options, common.mustNotCall());
8283
req.on('error', common.mustCall((err) => {
8384
assert.strictEqual(err.code, 'ABORT_ERR');
8485
assert.strictEqual(err.name, 'AbortError');
8586
server.close();
8687
}));
88+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
8789
assert.strictEqual(req.aborted, false);
8890
assert.strictEqual(req.destroyed, false);
8991
controller.abort();
9092
assert.strictEqual(req.aborted, false);
9193
assert.strictEqual(req.destroyed, true);
9294
}));
9395
}
96+
97+
{
98+
// Use post-abort async AbortSignal
99+
const server = http.createServer(common.mustNotCall());
100+
const controller = new AbortController();
101+
const { signal } = controller;
102+
server.listen(0, common.mustCall(() => {
103+
const options = { port: server.address().port, signal };
104+
const req = http.get(options, common.mustNotCall());
105+
req.on('error', common.mustCall((err) => {
106+
assert.strictEqual(err.code, 'ABORT_ERR');
107+
assert.strictEqual(err.name, 'AbortError');
108+
}));
109+
110+
req.on('close', common.mustCall(() => {
111+
assert.strictEqual(req.aborted, false);
112+
assert.strictEqual(req.destroyed, true);
113+
server.close();
114+
}));
115+
116+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
117+
process.nextTick(() => controller.abort());
118+
}));
119+
}
120+
121+
{
122+
// Use pre-aborted AbortSignal
123+
const server = http.createServer(common.mustNotCall());
124+
const controller = new AbortController();
125+
const { signal } = controller;
126+
server.listen(0, common.mustCall(() => {
127+
controller.abort();
128+
const options = { port: server.address().port, signal };
129+
const req = http.get(options, common.mustNotCall());
130+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
131+
req.on('error', common.mustCall((err) => {
132+
assert.strictEqual(err.code, 'ABORT_ERR');
133+
assert.strictEqual(err.name, 'AbortError');
134+
server.close();
135+
}));
136+
assert.strictEqual(req.aborted, false);
137+
assert.strictEqual(req.destroyed, true);
138+
}));
139+
}

‎test/parallel/test-http2-client-destroy.js‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@ if (!common.hasCrypto)
88
const assert = require('assert');
99
const h2 = require('http2');
1010
const { kSocket } = require('internal/http2/util');
11-
const { kEvents } = require('internal/event_target');
1211
const Countdown = require('../common/countdown');
13-
12+
const { getEventListeners } = require('events');
1413
{
1514
const server = h2.createServer();
1615
server.listen(0, common.mustCall(() => {
@@ -180,11 +179,11 @@ const Countdown = require('../common/countdown');
180179
client.on('close', common.mustCall());
181180

182181
const { signal } = controller;
183-
assert.strictEqual(signal[kEvents].get('abort'), undefined);
182+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
184183

185184
client.on('error', common.mustCall(() => {
186185
// After underlying stream dies, signal listener detached
187-
assert.strictEqual(signal[kEvents].get('abort'), undefined);
186+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
188187
}));
189188

190189
const req = client.request({}, { signal });
@@ -198,7 +197,7 @@ const Countdown = require('../common/countdown');
198197
assert.strictEqual(req.aborted, false);
199198
assert.strictEqual(req.destroyed, false);
200199
// Signal listener attached
201-
assert.strictEqual(signal[kEvents].get('abort').size, 1);
200+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
202201

203202
controller.abort();
204203

@@ -219,16 +218,16 @@ const Countdown = require('../common/countdown');
219218
const { signal } = controller;
220219
controller.abort();
221220

222-
assert.strictEqual(signal[kEvents].get('abort'), undefined);
221+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
223222

224223
client.on('error', common.mustCall(() => {
225224
// After underlying stream dies, signal listener detached
226-
assert.strictEqual(signal[kEvents].get('abort'), undefined);
225+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
227226
}));
228227

229228
const req = client.request({}, { signal });
230229
// Signal already aborted, so no event listener attached.
231-
assert.strictEqual(signal[kEvents].get('abort'), undefined);
230+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
232231

233232
assert.strictEqual(req.aborted, false);
234233
// Destroyed on same tick as request made

‎test/parallel/test-https-abortcontroller.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ if (!common.hasCrypto)
77
const fixtures = require('../common/fixtures');
88
const https = require('https');
99
const assert = require('assert');
10-
const { once } = require('events');
10+
const { once, getEventListeners } = require('events');
1111

1212
const options = {
1313
key: fixtures.readKey('agent1-key.pem'),
@@ -29,6 +29,7 @@ const options = {
2929
rejectUnauthorized: false,
3030
signal: ac.signal,
3131
});
32+
assert.strictEqual(getEventListeners(ac.signal, 'abort').length, 1);
3233
process.nextTick(() => ac.abort());
3334
const [ err ] = await once(req, 'error');
3435
assert.strictEqual(err.name, 'AbortError');

0 commit comments

Comments
 (0)