From ca5174c243a87939a0a5a09c0cce526f58589038 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Fri, 7 Apr 2023 20:50:30 +0200
Subject: [PATCH] fetch: fix leak

Fixes: https://github.com/nodejs/undici/issues/1823
---
 lib/fetch/request.js | 14 +++++++++++---
 package.json         |  2 +-
 test/fetch-leak.js   | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 test/fetch-leak.js

diff --git a/lib/fetch/request.js b/lib/fetch/request.js
index 080a5d7bfa3..60b99a4e158 100644
--- a/lib/fetch/request.js
+++ b/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,12 +355,19 @@ 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 weakAc = new WeakRef(ac)
         const abort = function () {
-          ac.abort(this.reason)
+          weakAc.deref()?.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)
@@ -367,7 +375,7 @@ class Request {
         } catch {}
 
         signal.addEventListener('abort', abort, { once: true })
-        requestFinalizer.register(this, { signal, abort })
+        requestFinalizer.register(ac, { signal, abort })
       }
     }
 
diff --git a/package.json b/package.json
index e019ebc7d1e..fc2f24a092e 100644
--- a/package.json
+++ b/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",
diff --git a/test/fetch-leak.js b/test/fetch-leak.js
new file mode 100644
index 00000000000..3d06a6e133e
--- /dev/null
+++ b/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))
+})