From c9040482d4226f52ecaab44b379c5f4911eae53b Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Mon, 28 Oct 2024 12:53:11 +0000 Subject: [PATCH 1/2] use AbortSignal.any if possible --- lib/web/fetch/request.js | 108 +++++++++++++--------- test/fetch/issue-node-46525.js | 7 +- test/fetch/long-lived-abort-controller.js | 7 +- test/fetch/max-listeners.js | 7 +- test/fetch/request.js | 34 +++---- 5 files changed, 92 insertions(+), 71 deletions(-) diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 97fea22cdbd..53d59d54d49 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -29,6 +29,9 @@ const { kConstruct } = require('../../core/symbols') const assert = require('node:assert') const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node:events') +// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf +const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23 + const kAbortController = Symbol('abortController') const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { @@ -407,39 +410,44 @@ class Request { // Realm. // TODO: could this be simplified with AbortSignal.any // (https://dom.spec.whatwg.org/#dom-abortsignal-any) - const ac = new AbortController() - this.#signal = ac.signal - - // 29. If signal is not null, then make this’s signal follow signal. - if (signal != null) { - if (signal.aborted) { - ac.abort(signal.reason) - } else { - // Keep a strong ref to ac while request object - // is alive. This is needed to prevent AbortController - // from being prematurely garbage collected. - // See, https://github.com/nodejs/undici/issues/1926. - this[kAbortController] = ac - - const acRef = new WeakRef(ac) - const abort = buildAbort(acRef) + if (isFixedOrderAbortSignalAny) { + // 29. If signal is not null, then make this’s signal follow signal. + this.#signal = AbortSignal.any(signal != null ? [signal] : []) + } else { + const ac = new AbortController() + this.#signal = ac.signal - // Third-party AbortControllers may not work with these. - // See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619. - try { - // If the max amount of listeners is equal to the default, increase it - // This is only available in node >= v19.9.0 - if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) { - setMaxListeners(1500, signal) - } - } catch {} - - util.addAbortListener(signal, abort) - // The third argument must be a registry key to be unregistered. - // Without it, you cannot unregister. - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry - // abort is used as the unregister key. (because it is unique) - requestFinalizer.register(ac, { signal, abort }, abort) + // 29. If signal is not null, then make this’s signal follow signal. + if (signal != null) { + if (signal.aborted) { + ac.abort(signal.reason) + } else { + // Keep a strong ref to ac while request object + // is alive. This is needed to prevent AbortController + // from being prematurely garbage collected. + // See, https://github.com/nodejs/undici/issues/1926. + this[kAbortController] = ac + + const acRef = new WeakRef(ac) + const abort = buildAbort(acRef) + + // Third-party AbortControllers may not work with these. + // See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619. + try { + // If the max amount of listeners is equal to the default, increase it + // This is only available in node >= v19.9.0 + if (typeof getMaxListeners === 'function' && getMaxListeners(signal) === defaultMaxListeners) { + setMaxListeners(1500, signal) + } + } catch {} + + util.addAbortListener(signal, abort) + // The third argument must be a registry key to be unregistered. + // Without it, you cannot unregister. + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry + // abort is used as the unregister key. (because it is unique) + requestFinalizer.register(ac, { signal, abort }, abort) + } } } @@ -771,25 +779,33 @@ class Request { // 3. Let clonedRequestObject be the result of creating a Request object, // given clonedRequest, this’s headers’s guard, and this’s relevant Realm. // 4. Make clonedRequestObject’s signal follow this’s signal. - const ac = new AbortController() - if (this.signal.aborted) { - ac.abort(this.signal.reason) + let signal + + if (isFixedOrderAbortSignalAny) { + signal = AbortSignal.any([this.#signal]) } else { - let list = dependentControllerMap.get(this.signal) - if (list === undefined) { - list = new Set() - dependentControllerMap.set(this.signal, list) + const ac = new AbortController() + if (this.#signal.aborted) { + ac.abort(this.#signal.reason) + } else { + let list = dependentControllerMap.get(this.#signal) + if (list === undefined) { + list = new Set() + dependentControllerMap.set(this.#signal, list) + } + const acRef = new WeakRef(ac) + list.add(acRef) + util.addAbortListener( + ac.signal, + buildAbort(acRef) + ) } - const acRef = new WeakRef(ac) - list.add(acRef) - util.addAbortListener( - ac.signal, - buildAbort(acRef) - ) + + signal = ac.signal } // 4. Return clonedRequestObject. - return fromInnerRequest(clonedRequest, this.#dispatcher, ac.signal, getHeadersGuard(this.#headers)) + return fromInnerRequest(clonedRequest, this.#dispatcher, signal, getHeadersGuard(this.#headers)) } [nodeUtil.inspect.custom] (depth, options) { diff --git a/test/fetch/issue-node-46525.js b/test/fetch/issue-node-46525.js index b35eeb2a590..4c0a62acfc3 100644 --- a/test/fetch/issue-node-46525.js +++ b/test/fetch/issue-node-46525.js @@ -4,9 +4,14 @@ const { once } = require('node:events') const { createServer } = require('node:http') const { test } = require('node:test') const { fetch } = require('../..') +const util = require('../../lib/core/util') +// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf +const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23 + +// TODO: Drop support below node v23, then delete this. // https://github.com/nodejs/node/issues/46525 -test('No warning when reusing AbortController', async (t) => { +test('No warning when reusing AbortController', { skip: isFixedOrderAbortSignalAny }, async (t) => { function onWarning () { throw new Error('Got warning') } diff --git a/test/fetch/long-lived-abort-controller.js b/test/fetch/long-lived-abort-controller.js index 1518989e4fb..69fe12a87a6 100644 --- a/test/fetch/long-lived-abort-controller.js +++ b/test/fetch/long-lived-abort-controller.js @@ -6,10 +6,15 @@ const { once } = require('events') const { test } = require('node:test') const { closeServerAsPromise } = require('../utils/node-http') const { strictEqual } = require('node:assert') +const util = require('../../lib/core/util') + +// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf +const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23 const isNode18 = process.version.startsWith('v18') -test('long-lived-abort-controller', { skip: isNode18 }, async (t) => { +// TODO: Drop support below node v23, then delete this. +test('long-lived-abort-controller', { skip: isNode18 || isFixedOrderAbortSignalAny }, async (t) => { const server = http.createServer((req, res) => { res.writeHead(200, { 'Content-Type': 'text/plain' }) res.write('Hello World!') diff --git a/test/fetch/max-listeners.js b/test/fetch/max-listeners.js index 36e4fef101b..0c8b7a088f4 100644 --- a/test/fetch/max-listeners.js +++ b/test/fetch/max-listeners.js @@ -4,8 +4,13 @@ const { setMaxListeners, getMaxListeners, defaultMaxListeners } = require('event const { test } = require('node:test') const assert = require('node:assert') const { Request } = require('../..') +const util = require('../../lib/core/util') -test('test max listeners', (t) => { +// https://github.com/nodejs/node/commit/d4736060404726a24d4e52647b8c9b88914b8ddf +const isFixedOrderAbortSignalAny = typeof AbortSignal.any === 'function' && util.nodeMajor >= 23 + +// TODO: Drop support below node v23, then delete this. +test('test max listeners', { skip: isFixedOrderAbortSignalAny }, (t) => { const controller = new AbortController() setMaxListeners(Infinity, controller.signal) for (let i = 0; i <= defaultMaxListeners; i++) { diff --git a/test/fetch/request.js b/test/fetch/request.js index 8ee4c8ed06d..af6a36f2d24 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -11,8 +11,6 @@ const { fetch } = require('../../') -const hasSignalReason = 'reason' in AbortSignal.prototype - test('arg validation', async (t) => { // constructor assert.throws(() => { @@ -261,25 +259,21 @@ test('pre aborted signal', () => { ac.abort('gwak') const req = new Request('http://asd', { signal: ac.signal }) assert.strictEqual(req.signal.aborted, true) - if (hasSignalReason) { - assert.strictEqual(req.signal.reason, 'gwak') - } + assert.strictEqual(req.signal.reason, 'gwak') }) -test('post aborted signal', (t) => { - const { strictEqual, ok } = tspl(t, { plan: 2 }) +test('post aborted signal', async (t) => { + const { strictEqual, completed } = tspl(t, { plan: 2 }) const ac = new AbortController() const req = new Request('http://asd', { signal: ac.signal }) strictEqual(req.signal.aborted, false) ac.signal.addEventListener('abort', () => { - if (hasSignalReason) { - strictEqual(req.signal.reason, 'gwak') - } else { - ok(true) - } + strictEqual(req.signal.reason, 'gwak') }, { once: true }) ac.abort('gwak') + + await completed }) test('pre aborted signal cloned', () => { @@ -287,9 +281,7 @@ test('pre aborted signal cloned', () => { ac.abort('gwak') const req = new Request('http://asd', { signal: ac.signal }).clone() assert.strictEqual(req.signal.aborted, true) - if (hasSignalReason) { - assert.strictEqual(req.signal.reason, 'gwak') - } + assert.strictEqual(req.signal.reason, 'gwak') }) test('URLSearchParams body with Headers object - issue #1407', async () => { @@ -313,20 +305,18 @@ test('URLSearchParams body with Headers object - issue #1407', async () => { assert.strictEqual(await request.text(), 'abc=123') }) -test('post aborted signal cloned', (t) => { - const { strictEqual, ok } = tspl(t, { plan: 2 }) +test('post aborted signal cloned', async (t) => { + const { strictEqual, completed } = tspl(t, { plan: 2 }) const ac = new AbortController() const req = new Request('http://asd', { signal: ac.signal }).clone() strictEqual(req.signal.aborted, false) ac.signal.addEventListener('abort', () => { - if (hasSignalReason) { - strictEqual(req.signal.reason, 'gwak') - } else { - ok(true) - } + strictEqual(req.signal.reason, 'gwak') }, { once: true }) ac.abort('gwak') + + await completed }) test('Passing headers in init', async (t) => { From 906a88e96084699c5f4d3ab3a3e62f6ec503cb3c Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Mon, 28 Oct 2024 12:55:55 +0000 Subject: [PATCH 2/2] lint fix --- lib/web/fetch/request.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 53d59d54d49..7c53f4d99e5 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -440,7 +440,7 @@ class Request { setMaxListeners(1500, signal) } } catch {} - + util.addAbortListener(signal, abort) // The third argument must be a registry key to be unregistered. // Without it, you cannot unregister.