fetch: do not leak signal listeners (#3158)

* Do not leak signal listeners

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* fixup

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* Update lib/web/fetch/request.js

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>

---------

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>

lib/web/fetch/dispatcher-weakref.js

@@ -33,9 +33,10 @@ class CompatFinalizer {
 }
 
 module.exports = function () {
-  // FIXME: remove workaround when the Node bug is fixed
+  // FIXME: remove workaround when the Node bug is backported to v18
   // https://github.com/nodejs/node/issues/49344#issuecomment-1741776308
-  if (process.env.NODE_V8_COVERAGE) {
+  if (process.env.NODE_V8_COVERAGE && process.version.startsWith('v18')) {
+    process._rawDebug('Using compatibility WeakRef and FinalizationRegistry')
     return {
       WeakRef: CompatWeakRef,
       FinalizationRegistry: CompatFinalizer

lib/web/fetch/request.js

@@ -38,6 +38,29 @@ const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {
   signal.removeEventListener('abort', abort)
 })
 
+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)
+    }
+  }
+}
+
 let patchMethodWarning = false
 
 // https://fetch.spec.whatwg.org/#request-class
@@ -377,34 +400,17 @@ class Request {
         this[kAbortController] = ac
 
         const acRef = new WeakRef(ac)
-        const abort = function () {
-          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 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(100, signal)
+            setMaxListeners(1500, signal)
           } else if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) {
-            setMaxListeners(100, signal)
+            setMaxListeners(1500, signal)
           }
         } catch {}
 

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

@@ -0,0 +1,48 @@
+'use strict'
+
+const http = require('node:http')
+const { fetch } = require('../../')
+const { once } = require('events')
+const { test } = require('node:test')
+const { closeServerAsPromise } = require('../utils/node-http')
+const { strictEqual } = require('node:assert')
+
+const isNode18 = process.version.startsWith('v18')
+
+test('long-lived-abort-controller', { skip: isNode18 }, async (t) => {
+  const server = http.createServer((req, res) => {
+    res.writeHead(200, { 'Content-Type': 'text/plain' })
+    res.write('Hello World!')
+    res.end()
+  }).listen(0)
+
+  await once(server, 'listening')
+
+  t.after(closeServerAsPromise(server))
+
+  let warningEmitted = false
+  function onWarning () {
+    warningEmitted = true
+  }
+  process.on('warning', onWarning)
+  t.after(() => {
+    process.off('warning', onWarning)
+  })
+
+  const controller = new AbortController()
+
+  // The maxListener is set to 1500 in request.js.
+  // we set it to 2000 to make sure that we are not leaking event listeners.
+  // Unfortunately we are relying on GC and implementation details here.
+  for (let i = 0; i < 2000; i++) {
+    // make request
+    const res = await fetch(`http://localhost:${server.address().port}`, {
+      signal: controller.signal
+    })
+
+    // drain body
+    await res.text()
+  }
+
+  strictEqual(warningEmitted, false)
+})