fetch: fix leak (#2049)

Author: ronag

Diff for: 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 })
       }
     }
 

Diff for: package.json

@@ -49,7 +49,7 @@
     "test": "npm run test:tap && npm run test:node-fetch && npm run test:fetch && npm run test:cookies && npm run test:wpt && npm run test:websocket && npm run test:jest && tsd",
     "test:cookies": "node scripts/verifyVersion 16 || tap test/cookie/*.js",
     "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:fetch": "node scripts/verifyVersion.js 16 || (npm run build:node && tap --expose-gc 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:tdd": "tap test/*.js test/diagnostics-channel/*.js -w",

Diff for: test/client-keep-alive.js

@@ -32,7 +32,7 @@ test('keep-alive header', (t) => {
       body.on('end', () => {
         const timeout = setTimeout(() => {
           t.fail()
-        }, 3e3)
+        }, 4e3)
         client.on('disconnect', () => {
           t.pass()
           clearTimeout(timeout)

Diff for: test/fetch/fetch-leak.js

@@ -0,0 +1,43 @@
+'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)
+  })
+
+  let prev = Infinity
+  let count = 0
+  const interval = setInterval(() => {
+    done = true
+    global.gc()
+    const next = process.memoryUsage().heapUsed
+    if (next <= prev) {
+      t.pass()
+    } else if (count++ > 10) {
+      t.fail()
+    } else {
+      prev = next
+    }
+  }, 1e3)
+  t.teardown(() => clearInterval(interval))
+})