Skip to content

Commit c904048

Browse files
committed
use AbortSignal.any if possible
1 parent 2de0f34 commit c904048

5 files changed

Lines changed: 92 additions & 71 deletions

File tree

lib/web/fetch/request.js

Lines changed: 62 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ const { kConstruct } = require('../../core/symbols')
2929
const assert = require('node:assert')
3030
const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node:events')
3131

32+
// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
33+
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23
34+
3235
const kAbortController = Symbol('abortController')
3336

3437
const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {
@@ -407,39 +410,44 @@ class Request {
407410
// Realm.
408411
// TODO: could this be simplified with AbortSignal.any
409412
// (https://dom.spec.whatwg.org/#dom-abortsignal-any)
410-
const ac = new AbortController()
411-
this.#signal = ac.signal
412-
413-
// 29. If signal is not null, then make this’s signal follow signal.
414-
if (signal != null) {
415-
if (signal.aborted) {
416-
ac.abort(signal.reason)
417-
} else {
418-
// Keep a strong ref to ac while request object
419-
// is alive. This is needed to prevent AbortController
420-
// from being prematurely garbage collected.
421-
// See, https://github.com/nodejs/undici/issues/1926.
422-
this[kAbortController] = ac
423-
424-
const acRef = new WeakRef(ac)
425-
const abort = buildAbort(acRef)
413+
if (isFixedOrderAbortSignalAny) {
414+
// 29. If signal is not null, then make this’s signal follow signal.
415+
this.#signal = AbortSignal.any(signal != null ? [signal] : [])
416+
} else {
417+
const ac = new AbortController()
418+
this.#signal = ac.signal
426419

427-
// Third-party AbortControllers may not work with these.
428-
// See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
429-
try {
430-
// If the max amount of listeners is equal to the default, increase it
431-
// This is only available in node >= v19.9.0
432-
if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) {
433-
setMaxListeners(1500, signal)
434-
}
435-
} catch {}
436-
437-
util.addAbortListener(signal, abort)
438-
// The third argument must be a registry key to be unregistered.
439-
// Without it, you cannot unregister.
440-
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
441-
// abort is used as the unregister key. (because it is unique)
442-
requestFinalizer.register(ac, { signal, abort }, abort)
420+
// 29. If signal is not null, then make this’s signal follow signal.
421+
if (signal != null) {
422+
if (signal.aborted) {
423+
ac.abort(signal.reason)
424+
} else {
425+
// Keep a strong ref to ac while request object
426+
// is alive. This is needed to prevent AbortController
427+
// from being prematurely garbage collected.
428+
// See, https://github.com/nodejs/undici/issues/1926.
429+
this[kAbortController] = ac
430+
431+
const acRef = new WeakRef(ac)
432+
const abort = buildAbort(acRef)
433+
434+
// Third-party AbortControllers may not work with these.
435+
// See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
436+
try {
437+
// If the max amount of listeners is equal to the default, increase it
438+
// This is only available in node >= v19.9.0
439+
if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) {
440+
setMaxListeners(1500, signal)
441+
}
442+
} catch {}
443+
444+
util.addAbortListener(signal, abort)
445+
// The third argument must be a registry key to be unregistered.
446+
// Without it, you cannot unregister.
447+
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
448+
// abort is used as the unregister key. (because it is unique)
449+
requestFinalizer.register(ac, { signal, abort }, abort)
450+
}
443451
}
444452
}
445453

@@ -771,25 +779,33 @@ class Request {
771779
// 3. Let clonedRequestObject be the result of creating a Request object,
772780
// given clonedRequest, this’s headers’s guard, and this’s relevant Realm.
773781
// 4. Make clonedRequestObject’s signal follow this’s signal.
774-
const ac = new AbortController()
775-
if (this.signal.aborted) {
776-
ac.abort(this.signal.reason)
782+
let signal
783+
784+
if (isFixedOrderAbortSignalAny) {
785+
signal = AbortSignal.any([this.#signal])
777786
} else {
778-
let list = dependentControllerMap.get(this.signal)
779-
if (list === undefined) {
780-
list = new Set()
781-
dependentControllerMap.set(this.signal, list)
787+
const ac = new AbortController()
788+
if (this.#signal.aborted) {
789+
ac.abort(this.#signal.reason)
790+
} else {
791+
let list = dependentControllerMap.get(this.#signal)
792+
if (list === undefined) {
793+
list = new Set()
794+
dependentControllerMap.set(this.#signal, list)
795+
}
796+
const acRef = new WeakRef(ac)
797+
list.add(acRef)
798+
util.addAbortListener(
799+
ac.signal,
800+
buildAbort(acRef)
801+
)
782802
}
783-
const acRef = new WeakRef(ac)
784-
list.add(acRef)
785-
util.addAbortListener(
786-
ac.signal,
787-
buildAbort(acRef)
788-
)
803+
804+
signal = ac.signal
789805
}
790806

791807
// 4. Return clonedRequestObject.
792-
return fromInnerRequest(clonedRequest, this.#dispatcher, ac.signal, getHeadersGuard(this.#headers))
808+
return fromInnerRequest(clonedRequest, this.#dispatcher, signal, getHeadersGuard(this.#headers))
793809
}
794810

795811
[nodeUtil.inspect.custom] (depth, options) {

test/fetch/issue-node-46525.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,14 @@ const { once } = require('node:events')
44
const { createServer } = require('node:http')
55
const { test } = require('node:test')
66
const { fetch } = require('../..')
7+
const util = require('../../lib/core/util')
78

9+
// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
10+
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23
11+
12+
// TODO: Drop support below node v23, then delete this.
813
// https://github.com/nodejs/node/issues/46525
9-
test('No warning when reusing AbortController', async (t) => {
14+
test('No warning when reusing AbortController', { skip: isFixedOrderAbortSignalAny }, async (t) => {
1015
function onWarning () {
1116
throw new Error('Got warning')
1217
}

test/fetch/long-lived-abort-controller.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,15 @@ const { once } = require('events')
66
const { test } = require('node:test')
77
const { closeServerAsPromise } = require('../utils/node-http')
88
const { strictEqual } = require('node:assert')
9+
const util = require('../../lib/core/util')
10+
11+
// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
12+
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23
913

1014
const isNode18 = process.version.startsWith('v18')
1115

12-
test('long-lived-abort-controller', { skip: isNode18 }, async (t) => {
16+
// TODO: Drop support below node v23, then delete this.
17+
test('long-lived-abort-controller', { skip: isNode18 || isFixedOrderAbortSignalAny }, async (t) => {
1318
const server = http.createServer((req, res) => {
1419
res.writeHead(200, { 'Content-Type': 'text/plain' })
1520
res.write('Hello World!')

test/fetch/max-listeners.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,13 @@ const { setMaxListeners, getMaxListeners, defaultMaxListeners } = require('event
44
const { test } = require('node:test')
55
const assert = require('node:assert')
66
const { Request } = require('../..')
7+
const util = require('../../lib/core/util')
78

8-
test('test max listeners', (t) => {
9+
// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf
10+
const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23
11+
12+
// TODO: Drop support below node v23, then delete this.
13+
test('test max listeners', { skip: isFixedOrderAbortSignalAny }, (t) => {
914
const controller = new AbortController()
1015
setMaxListeners(Infinity, controller.signal)
1116
for (let i = 0; i <= defaultMaxListeners; i++) {

test/fetch/request.js

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ const {
1111
fetch
1212
} = require('../../')
1313

14-
const hasSignalReason = 'reason' in AbortSignal.prototype
15-
1614
test('arg validation', async (t) => {
1715
// constructor
1816
assert.throws(() => {
@@ -261,35 +259,29 @@ test('pre aborted signal', () => {
261259
ac.abort('gwak')
262260
const req = new Request('http://asd', { signal: ac.signal })
263261
assert.strictEqual(req.signal.aborted, true)
264-
if (hasSignalReason) {
265-
assert.strictEqual(req.signal.reason, 'gwak')
266-
}
262+
assert.strictEqual(req.signal.reason, 'gwak')
267263
})
268264

269-
test('post aborted signal', (t) => {
270-
const { strictEqual, ok } = tspl(t, { plan: 2 })
265+
test('post aborted signal', async (t) => {
266+
const { strictEqual, completed } = tspl(t, { plan: 2 })
271267

272268
const ac = new AbortController()
273269
const req = new Request('http://asd', { signal: ac.signal })
274270
strictEqual(req.signal.aborted, false)
275271
ac.signal.addEventListener('abort', () => {
276-
if (hasSignalReason) {
277-
strictEqual(req.signal.reason, 'gwak')
278-
} else {
279-
ok(true)
280-
}
272+
strictEqual(req.signal.reason, 'gwak')
281273
}, { once: true })
282274
ac.abort('gwak')
275+
276+
await completed
283277
})
284278

285279
test('pre aborted signal cloned', () => {
286280
const ac = new AbortController()
287281
ac.abort('gwak')
288282
const req = new Request('http://asd', { signal: ac.signal }).clone()
289283
assert.strictEqual(req.signal.aborted, true)
290-
if (hasSignalReason) {
291-
assert.strictEqual(req.signal.reason, 'gwak')
292-
}
284+
assert.strictEqual(req.signal.reason, 'gwak')
293285
})
294286

295287
test('URLSearchParams body with Headers object - issue #1407', async () => {
@@ -313,20 +305,18 @@ test('URLSearchParams body with Headers object - issue #1407', async () => {
313305
assert.strictEqual(await request.text(), 'abc=123')
314306
})
315307

316-
test('post aborted signal cloned', (t) => {
317-
const { strictEqual, ok } = tspl(t, { plan: 2 })
308+
test('post aborted signal cloned', async (t) => {
309+
const { strictEqual, completed } = tspl(t, { plan: 2 })
318310

319311
const ac = new AbortController()
320312
const req = new Request('http://asd', { signal: ac.signal }).clone()
321313
strictEqual(req.signal.aborted, false)
322314
ac.signal.addEventListener('abort', () => {
323-
if (hasSignalReason) {
324-
strictEqual(req.signal.reason, 'gwak')
325-
} else {
326-
ok(true)
327-
}
315+
strictEqual(req.signal.reason, 'gwak')
328316
}, { once: true })
329317
ac.abort('gwak')
318+
319+
await completed
330320
})
331321

332322
test('Passing headers in init', async (t) => {

0 commit comments

Comments
 (0)