diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 6ef40f99920..474796e1e1e 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -26,60 +26,13 @@ const { webidl } = require('../webidl') const { URLSerializer } = require('./data-url') const { kConstruct } = require('../../core/symbols') const assert = require('node:assert') -const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node:events') -const kAbortController = Symbol('abortController') - -const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { - signal.removeEventListener('abort', abort) -}) - -const dependentControllerMap = new WeakMap() - -let abortSignalHasEventHandlerLeakWarning - -try { - abortSignalHasEventHandlerLeakWarning = getMaxListeners(new AbortController().signal) > 0 -} catch { - abortSignalHasEventHandlerLeakWarning = false -} - -function buildAbort (acRef) { - return abort - - function abort () { - const ac = acRef.deref() - if (ac !== undefined) { - // Currently, there is a problem with FinalizationRegistry. - // https://github.com/nodejs/node/issues/49344 - // https://github.com/nodejs/node/issues/47748 - // In the case of abort, the first step is to unregister from it. - // If the controller can refer to it, it is still registered. - // It will be removed in the future. - requestFinalizer.unregister(abort) - - // Unsubscribe a listener. - // FinalizationRegistry will no longer be called, so this must be done. - this.removeEventListener('abort', abort) - - ac.abort(this.reason) - - const controllerList = dependentControllerMap.get(ac.signal) - - if (controllerList !== undefined) { - if (controllerList.size !== 0) { - for (const ref of controllerList) { - const ctrl = ref.deref() - if (ctrl !== undefined) { - ctrl.abort(this.reason) - } - } - controllerList.clear() - } - dependentControllerMap.delete(ac.signal) - } - } +function makeRequestSignal (signal) { + if (signal == null) { + return new AbortController().signal } + + return AbortSignal.any([signal]) } let patchMethodWarning = false @@ -412,38 +365,7 @@ class Request { // 28. Set this’s signal to a new AbortSignal object with this’s relevant // 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 the max amount of listeners is equal to the default, increase it - if (abortSignalHasEventHandlerLeakWarning && getMaxListeners(signal) === defaultMaxListeners) { - setMaxListeners(1500, signal) - } - - 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) - } - } + this.#signal = makeRequestSignal(signal) // 30. Set this’s headers to a new Headers object with this’s relevant // Realm, whose header list is request’s header list and guard is @@ -773,25 +695,10 @@ 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) - } 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 signal = makeRequestSignal(this.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/request.js b/test/fetch/request.js index 51e92b5c061..58af268827d 100644 --- a/test/fetch/request.js +++ b/test/fetch/request.js @@ -2,6 +2,7 @@ 'use strict' +const { setMaxListeners } = require('node:events') const { test } = require('node:test') const { Request, @@ -9,6 +10,30 @@ const { fetch } = require('../../') +async function forceGarbageCollection (iterations = 50) { + for (let i = 0; i < iterations; ++i) { + const garbage = Array.from({ length: 1000 }, () => ({ value: Math.random() })) + if (garbage.length === 0) { + throw new Error('unreachable') + } + global.gc() + await new Promise((resolve) => setTimeout(resolve, 0)) + } +} + +function waitForAbort (signal, timeout = 1000) { + return new Promise((resolve) => { + const timer = setTimeout(() => { + resolve(false) + }, timeout) + + signal.addEventListener('abort', () => { + clearTimeout(timer) + resolve(true) + }, { once: true }) + }) +} + test('arg validation', async (t) => { // constructor t.assert.throws(() => { @@ -313,6 +338,82 @@ test('post aborted signal cloned', (t) => { ac.abort('gwak') }) +test('request signal stays connected after the request is garbage collected', async (t) => { + if (typeof global.gc !== 'function') { + t.skip('gc is not available. Run with --expose-gc.') + return + } + + const ac = new AbortController() + let signal + + { + const request = new Request('http://asd', { signal: ac.signal }) + signal = request.signal + } + + const aborted = waitForAbort(signal) + + await forceGarbageCollection() + + ac.abort('gwak') + t.assert.strictEqual(await aborted, true) + t.assert.strictEqual(signal.aborted, true) + t.assert.strictEqual(signal.reason, 'gwak') +}) + +test('cloned request signal stays connected after garbage collection', async (t) => { + if (typeof global.gc !== 'function') { + t.skip('gc is not available. Run with --expose-gc.') + return + } + + const ac = new AbortController() + let request + + { + const originalRequest = new Request('http://asd', { signal: ac.signal }) + request = originalRequest.clone() + } + + const aborted = waitForAbort(request.signal) + + await forceGarbageCollection() + + ac.abort('gwak') + t.assert.strictEqual(await aborted, true) + t.assert.strictEqual(request.signal.aborted, true) + t.assert.strictEqual(request.signal.reason, 'gwak') +}) + +test('reusing a controller across transient requests does not emit a warning', async (t) => { + if (typeof global.gc !== 'function') { + t.skip('gc is not available. Run with --expose-gc.') + return + } + + let emittedWarning = '' + function onWarning (warning) { + emittedWarning = warning + } + + process.on('warning', onWarning) + t.after(() => { + process.off('warning', onWarning) + }) + + const controller = new AbortController() + setMaxListeners(20, controller.signal) + + for (let i = 0; i < 200; ++i) { + const request = new Request('http://asd', { signal: controller.signal }) + request.signal.addEventListener('abort', () => {}, { once: true }) + await forceGarbageCollection(1) + } + + t.assert.strictEqual(emittedWarning, '') +}) + test('Passing headers in init', async (t) => { // https://github.com/nodejs/undici/issues/1400 await t.test('Headers instance', (t) => {