Skip to content

Commit 43d4392

Browse files
LinkgoronLinkgoron
committed
http: fix double AbortSignal registration
Fix an issue where AbortSignals are registered twice
1 parent b9fd4eb commit 43d4392

5 files changed

+176
-16
lines changed

‎lib/_http_client.js

+12-5
Original file line numberDiff line numberDiff line change
@@ -288,14 +288,20 @@ function ClientRequest(input, options, cb) {
288288
options.headers);
289289
}
290290

291+
let optsWithoutSignal = options;
292+
if (optsWithoutSignal.signal) {
293+
optsWithoutSignal = ObjectAssign({}, options);
294+
delete optsWithoutSignal.signal;
295+
}
296+
291297
// initiate connection
292298
if (this.agent) {
293-
this.agent.addRequest(this, options);
299+
this.agent.addRequest(this, optsWithoutSignal);
294300
} else {
295301
// No agent, default to Connection:close.
296302
this._last = true;
297303
this.shouldKeepAlive = false;
298-
if (typeof options.createConnection === 'function') {
304+
if (typeof optsWithoutSignal.createConnection === 'function') {
299305
const oncreate = once((err, socket) => {
300306
if (err) {
301307
process.nextTick(() => emitError(this, err));
@@ -305,16 +311,17 @@ function ClientRequest(input, options, cb) {
305311
});
306312

307313
try {
308-
const newSocket = options.createConnection(options, oncreate);
314+
const newSocket = optsWithoutSignal.createConnection(optsWithoutSignal,
315+
oncreate);
309316
if (newSocket) {
310317
oncreate(null, newSocket);
311318
}
312319
} catch (err) {
313320
oncreate(err);
314321
}
315322
} else {
316-
debug('CLIENT use net.createConnection', options);
317-
this.onSocket(net.createConnection(options));
323+
debug('CLIENT use net.createConnection', optsWithoutSignal);
324+
this.onSocket(net.createConnection(optsWithoutSignal));
318325
}
319326
}
320327
}

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

+93
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
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 } = 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+
function postCreateConnection() {
20+
return new Promise((res) => {
21+
const ac = new AbortController();
22+
const { signal } = ac;
23+
const connection = agent.createConnection({ ...options, signal });
24+
connection.on('error', common.mustCall((err) => {
25+
assert(err);
26+
assert.strictEqual(err.name, 'AbortError');
27+
res();
28+
}));
29+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
30+
ac.abort();
31+
});
32+
}
33+
34+
function preCreateConnection() {
35+
return new Promise((res) => {
36+
const ac = new AbortController();
37+
const { signal } = ac;
38+
ac.abort();
39+
const connection = agent.createConnection({ ...options, signal });
40+
connection.on('error', common.mustCall((err) => {
41+
assert(err);
42+
assert.strictEqual(err.name, 'AbortError');
43+
res();
44+
}));
45+
});
46+
}
47+
48+
function agentAsParam() {
49+
return new Promise((res) => {
50+
const ac = new AbortController();
51+
const { signal } = ac;
52+
const request = http.get({
53+
port: server.address().port,
54+
path: '/hello',
55+
agent: agent,
56+
signal,
57+
});
58+
request.on('error', common.mustCall((err) => {
59+
assert(err);
60+
assert.strictEqual(err.name, 'AbortError');
61+
res();
62+
}));
63+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
64+
ac.abort();
65+
});
66+
}
67+
68+
function agentAsParamPreAbort() {
69+
return new Promise((res) => {
70+
const ac = new AbortController();
71+
const { signal } = ac;
72+
ac.abort();
73+
const request = http.get({
74+
port: server.address().port,
75+
path: '/hello',
76+
agent: agent,
77+
signal,
78+
});
79+
request.on('error', common.mustCall((err) => {
80+
assert(err);
81+
assert.strictEqual(err.name, 'AbortError');
82+
res();
83+
}));
84+
assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
85+
});
86+
}
87+
88+
await postCreateConnection();
89+
await preCreateConnection();
90+
await agentAsParam();
91+
await agentAsParamPreAbort();
92+
server.close();
93+
}));

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

+23-2
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
@@ -75,19 +76,39 @@ const assert = require('assert');
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 pre-aborted AbortSignal
99+
const server = http.createServer(common.mustNotCall());
100+
const controller = new AbortController();
101+
const { signal } = controller;
102+
server.listen(0, common.mustCall(() => {
103+
controller.abort();
104+
const options = { port: server.address().port, signal };
105+
const req = http.get(options, common.mustNotCall());
106+
req.on('error', common.mustCall((err) => {
107+
assert.strictEqual(err.code, 'ABORT_ERR');
108+
assert.strictEqual(err.name, 'AbortError');
109+
server.close();
110+
}));
111+
assert.strictEqual(req.aborted, false);
112+
assert.strictEqual(req.destroyed, true);
113+
}));
114+
}

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

+46-8
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
@@ -241,3 +240,42 @@ const Countdown = require('../common/countdown');
241240
req.on('close', common.mustCall(() => server.close()));
242241
}));
243242
}
243+
244+
// Destroy ClientHttpSession with AbortSignal
245+
{
246+
const server = h2.createServer();
247+
const controller = new AbortController();
248+
249+
server.on('stream', common.mustNotCall());
250+
server.listen(0, common.mustCall(() => {
251+
const { signal } = controller;
252+
const client = h2.connect(`http://localhost:${server.address().port}`, {
253+
signal,
254+
});
255+
client.on('close', common.mustCall());
256+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
257+
258+
client.on('error', common.mustCall(common.mustCall((err) => {
259+
assert.strictEqual(err.code, 'ABORT_ERR');
260+
assert.strictEqual(err.name, 'AbortError');
261+
})));
262+
263+
const req = client.request({}, {});
264+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
265+
266+
req.on('error', common.mustCall((err) => {
267+
assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_CANCEL');
268+
assert.strictEqual(err.name, 'Error');
269+
assert.strictEqual(req.aborted, false);
270+
assert.strictEqual(req.destroyed, true);
271+
}));
272+
req.on('close', common.mustCall(() => server.close()));
273+
274+
assert.strictEqual(req.aborted, false);
275+
assert.strictEqual(req.destroyed, false);
276+
// Signal listener attached
277+
assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
278+
279+
controller.abort();
280+
}));
281+
}

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

+2-1
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)