fix: preserve Request signal abort propagation after GC

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) {

test/fetch/request.js

@@ -2,13 +2,38 @@
 
 'use strict'
 
+const { setMaxListeners } = require('node:events')
 const { test } = require('node:test')
 const {
   Request,
   Headers,
   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) => {