fetch: fix leak

Fixes: #1823
Fixes: nodejs/node#46435

lib/fetch/request.js

@@ -34,6 +34,7 @@ const { setMaxListeners, getEventListeners, defaultMaxListeners } = require('eve
 let TransformStream = globalThis.TransformStream
 
 const kInit = Symbol('init')
+const kAbortController = Symbol('abortController')
 
 const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {
   signal.removeEventListener('abort', abort)
@@ -354,20 +355,30 @@ class Request {
       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 = function () {
-          ac.abort(this.reason)
+          const ac = acRef.deref()
+          if (ac !== undefined) {
+            ac.abort(this.reason)
+          }
         }
 
         // Third-party AbortControllers may not work with these.
-        // See https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619
+        // See, https://github.com/nodejs/undici/pull/1910#issuecomment-1464495619.
         try {
           if (getEventListeners(signal, 'abort').length >= defaultMaxListeners) {
             setMaxListeners(100, signal)
           }
         } catch {}
 
         signal.addEventListener('abort', abort, { once: true })
-        requestFinalizer.register(this, { signal, abort })
+        requestFinalizer.register(ac, { signal, abort })
       }
     }
 

package.json

@@ -51,7 +51,7 @@
     "test:node-fetch": "node scripts/verifyVersion.js 16 || mocha test/node-fetch",
     "test:fetch": "node scripts/verifyVersion.js 16 || (npm run build:node && tap test/fetch/*.js && tap test/webidl/*.js)",
     "test:jest": "node scripts/verifyVersion.js 14 || jest",
-    "test:tap": "tap test/*.js test/diagnostics-channel/*.js",
+    "test:tap": "tap --expose-gc test/*.js test/diagnostics-channel/*.js",
     "test:tdd": "tap test/*.js test/diagnostics-channel/*.js -w",
     "test:typescript": "tsd && tsc test/imports/undici-import.ts",
     "test:websocket": "node scripts/verifyVersion.js 18 || tap test/websocket/*.js",

test/fetch-leak.js

@@ -0,0 +1,40 @@
+'use strict'
+
+const { test } = require('tap')
+const { fetch } = require('..')
+const { createServer } = require('http')
+
+test('do not leak', (t) => {
+  t.plan(1)
+
+  const server = createServer((req, res) => {
+    res.end()
+  })
+  t.teardown(server.close.bind(server))
+
+  let url
+  let done = false
+  server.listen(0, function attack () {
+    if (done) {
+      return
+    }
+    url ??= new URL(`http://127.0.0.1:${server.address().port}`)
+    const controller = new AbortController()
+    fetch(url, { signal: controller.signal })
+      .then(res => res.arrayBuffer())
+      .then(attack)
+  })
+
+  const xs = []
+  const interval = setInterval(() => {
+    global.gc()
+    xs.push(process.memoryUsage().heapUsed)
+    if (xs.length > 5) {
+      done = true
+      const final = xs.pop() // compare against final value
+      xs.splice(2) // skip first two values, memory can still be growing
+      t.ok(xs.every(x => final - x < 1e6))
+    }
+  }, 1e3)
+  t.teardown(() => clearInterval(interval))
+})