diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 66f8150cb82..2c3252bb6a5 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -30,8 +30,12 @@ const { getMaxListeners, setMaxListeners, defaultMaxListeners } = require('node: const kAbortController = Symbol('abortController') -const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => { - signal.removeEventListener('abort', abort) +// The held value must not keep the caller's signal alive: if anything reachable +// from that signal can reach the Request, it would also keep the registry's +// target (the Request's AbortController) alive, and neither would ever be +// collected. A signal that is already gone has no listener left to remove. +const requestFinalizer = new FinalizationRegistry(({ signalRef, abort }) => { + signalRef.deref()?.removeEventListener('abort', abort) }) const dependentControllerMap = new WeakMap() @@ -449,7 +453,7 @@ class Request { // 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) + requestFinalizer.register(ac, { signalRef: new WeakRef(signal), abort }, abort) // Allow the listener to be removed deterministically once the fetch // that owns this request has settled, instead of relying solely on the diff --git a/test/fetch/request-signal-cycle.js b/test/fetch/request-signal-cycle.js new file mode 100644 index 00000000000..c6fb4fd65db --- /dev/null +++ b/test/fetch/request-signal-cycle.js @@ -0,0 +1,100 @@ +'use strict' + +const { test } = require('node:test') +const assert = require('node:assert') +const { setImmediate: tick } = require('node:timers/promises') +const { Request } = require('../..') + +const hasGC = typeof global.gc !== 'undefined' + +// A Request that follows a caller's signal is registered in a +// FinalizationRegistry keyed on its own AbortController. The held value used to +// keep a strong reference to the caller's signal. When anything reachable from +// that signal could reach the Request (a listener or a property closing over +// it), the held value kept the registry's own target alive, so neither the +// Request nor anything it held was ever collected. +async function collected (ref) { + for (let i = 0; i < 10 && ref.deref() !== undefined; i++) { + await tick() + global.gc() + } + return ref.deref() === undefined +} + +test('a Request is collected when a listener on its signal references it', async () => { + if (!hasGC) { + throw new Error('gc is not available. Run with \'--expose-gc\'.') + } + + let ref + ;(() => { + const controller = new AbortController() + const request = new Request('http://localhost', { signal: controller.signal }) + controller.signal.addEventListener('abort', () => request.url) + ref = new WeakRef(request) + })() + + assert.ok(await collected(ref)) +}) + +test('a Request is collected when a property of its signal references it', async () => { + if (!hasGC) { + throw new Error('gc is not available. Run with \'--expose-gc\'.') + } + + let ref + ;(() => { + const controller = new AbortController() + const request = new Request('http://localhost', { signal: controller.signal }) + controller.signal.cleanup = () => request.url + ref = new WeakRef(request) + })() + + assert.ok(await collected(ref)) +}) + +test('the abort listener is still removed when the Request is collected', async () => { + if (!hasGC) { + throw new Error('gc is not available. Run with \'--expose-gc\'.') + } + + const { getEventListeners } = require('node:events') + const controller = new AbortController() + let ref + ;(() => { + const request = new Request('http://localhost', { signal: controller.signal }) + ref = new WeakRef(request) + })() + + assert.ok(await collected(ref)) + // The registry's cleanup callback runs after the collection, on its own task. + for (let i = 0; i < 10 && getEventListeners(controller.signal, 'abort').length > 0; i++) { + await tick() + global.gc() + } + assert.strictEqual(getEventListeners(controller.signal, 'abort').length, 0) +}) + +test('a timeout signal nobody else references still aborts the Request', { timeout: 5000 }, async () => { + if (!hasGC) { + throw new Error('gc is not available. Run with \'--expose-gc\'.') + } + + // The held value no longer keeps the signal alive, so this relies on the + // signal keeping itself alive while it has an abort listener and can still fire. + const request = new Request('http://localhost', { signal: AbortSignal.timeout(100) }) + const aborted = new Promise((resolve) => request.signal.addEventListener('abort', resolve, { once: true })) + for (let i = 0; i < 10; i++) { + await tick() + global.gc() + } + + // The timeout's own timer is unref'd and would let the process exit first. + const keepAlive = setInterval(() => {}, 1000) + try { + await aborted + } finally { + clearInterval(keepAlive) + } + assert.strictEqual(request.signal.aborted, true) +})