Fix memory leak in `fetch(url)` (#2989)

* Fix memory leak in `fetch(url)`

* Bump those numbers up

---------

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>

4 files changed, 136 insertions, 11 deletions

diff --git a/src/bun.js/webcore/response.zig b/src/bun.js/webcore/response.zig
index e9e6e2df2..9d8f917bb 100644
--- a/src/bun.js/webcore/response.zig
+++ b/src/bun.js/webcore/response.zig
@@ -626,7 +626,7 @@ pub const Fetch = struct {
         request_body: AnyBlob = undefined,
         response_buffer: MutableString = undefined,
         request_headers: Headers = Headers{ .allocator = undefined },
-        ref: *JSC.napi.Ref = undefined,
+        promise: JSC.JSPromise.Strong,
         concurrent_task: JSC.ConcurrentTask = .{},
         poll_ref: JSC.PollRef = .{},
 
@@ -684,9 +684,10 @@ pub const Fetch = struct {
 
             const globalThis = this.global_this;
 
-            var ref = this.ref;
-            const promise_value = ref.get();
-            defer ref.destroy();
+            var ref = this.promise;
+            const promise_value = ref.value();
+            defer ref.strong.deinit();
+
             var poll_ref = this.poll_ref;
             var vm = globalThis.bunVM();
             defer poll_ref.unref(vm);
@@ -805,7 +806,7 @@ pub const Fetch = struct {
         pub fn get(
             allocator: std.mem.Allocator,
             globalThis: *JSC.JSGlobalObject,
-            promise: JSValue,
+            promise: JSC.JSPromise.Strong,
             fetch_options: FetchOptions,
         ) !*FetchTasklet {
             var jsc_vm = globalThis.bunVM();
@@ -824,7 +825,7 @@ pub const Fetch = struct {
                 .request_body = fetch_options.body,
                 .global_this = globalThis,
                 .request_headers = fetch_options.headers,
-                .ref = JSC.napi.Ref.create(globalThis, promise),
+                .promise = promise,
                 .url_proxy_buffer = fetch_options.url_proxy_buffer,
                 .signal = fetch_options.signal,
                 .hostname = fetch_options.hostname,
@@ -898,7 +899,7 @@ pub const Fetch = struct {
             allocator: std.mem.Allocator,
             global: *JSGlobalObject,
             fetch_options: FetchOptions,
-            promise: JSValue,
+            promise: JSC.JSPromise.Strong,
         ) !*FetchTasklet {
             try HTTPClient.HTTPThread.init();
             var node = try get(
@@ -968,6 +969,8 @@ pub const Fetch = struct {
 
         var url_proxy_buffer: []const u8 = undefined;
 
+        // TODO: move this into a DRYer implementation
+        // The status quo is very repetitive and very bug prone
         if (first_arg.as(Request)) |request| {
             if (args.nextEat()) |options| {
                 if (options.isObject() or options.jsType() == .DOMWrapper) {
@@ -1298,9 +1301,6 @@ pub const Fetch = struct {
             return .zero;
         }
 
-        var promise = JSPromise.Strong.init(globalThis);
-        var promise_val = promise.value();
-
         if (url.isEmpty()) {
             const err = JSC.toTypeError(.ERR_INVALID_ARG_VALUE, fetch_error_blank_url, .{}, ctx);
             return JSPromise.rejectedPromiseValue(globalThis, err);
@@ -1318,6 +1318,12 @@ pub const Fetch = struct {
             return JSPromise.rejectedPromiseValue(globalThis, err);
         }
 
+        // Only create this after we have validated all the input.
+        // or else we will leak it
+        var promise = JSPromise.Strong.init(globalThis);
+
+        const promise_val = promise.value();
+
         // var resolve = FetchTasklet.FetchResolver.Class.make(ctx: js.JSContextRef, ptr: *ZigType)
         _ = FetchTasklet.queue(
             default_allocator,
@@ -1340,8 +1346,12 @@ pub const Fetch = struct {
                 .globalThis = globalThis,
                 .hostname = hostname,
             },
-            promise_val,
+            // Pass the Strong value instead of creating a new one, or else we
+            // will leak it
+            // see https://github.com/oven-sh/bun/issues/2985
+            promise,
         ) catch unreachable;
+
         return promise_val;
     }
 };
diff --git a/test/js/web/fetch/fetch-leak-test-fixture-2.js b/test/js/web/fetch/fetch-leak-test-fixture-2.js
new file mode 100644
index 000000000..7379b1b7b
--- /dev/null
+++ b/test/js/web/fetch/fetch-leak-test-fixture-2.js
@@ -0,0 +1,26 @@
+import { heapStats } from "bun:jsc";
+
+const { SERVER } = process.env;
+
+if (typeof SERVER === "undefined" || !SERVER?.length) {
+  throw new Error("SERVER environment variable is not set");
+}
+
+const COUNT = parseInt(process.env.COUNT || "20", 10);
+var oks = 0;
+await (async function runAll() {
+  for (let j = 0; j < COUNT; j++) {
+    oks += (await fetch(SERVER)).ok;
+  }
+})();
+
+if (oks !== COUNT) {
+  throw new Error("Not all requests succeeded");
+}
+
+await Bun.sleep(10);
+Bun.gc(true);
+
+if ((heapStats().objectTypeCounts.Response ?? 0) > 5) {
+  throw new Error("Too many Response objects: " + heapStats().objectTypeCounts.Response);
+}
diff --git a/test/js/web/fetch/fetch-leak-test-fixture.js b/test/js/web/fetch/fetch-leak-test-fixture.js
new file mode 100644
index 000000000..07275a425
--- /dev/null
+++ b/test/js/web/fetch/fetch-leak-test-fixture.js
@@ -0,0 +1,34 @@
+import { heapStats } from "bun:jsc";
+
+const { SERVER } = process.env;
+
+if (typeof SERVER === "undefined" || !SERVER?.length) {
+  throw new Error("SERVER environment variable is not set");
+}
+
+const COUNT = parseInt(process.env.COUNT || "50", 10);
+await (async function runAll() {
+  var fetches = new Array(COUNT);
+  let i = 0;
+  while (i < Math.max(COUNT - 32, 0)) {
+    for (let j = 0; j < 32; j++) {
+      fetches.push(fetch(SERVER));
+    }
+    await Promise.all(fetches.slice(i, i + 32));
+    i += 32;
+  }
+
+  while (i++ < COUNT) {
+    fetches.push(fetch(SERVER));
+  }
+
+  await Promise.all(fetches);
+  fetches.length = 0;
+  fetches = [];
+})();
+await Bun.sleep(10);
+Bun.gc(true);
+
+if ((heapStats().objectTypeCounts.Response ?? 0) > 10) {
+  throw new Error("Too many Response objects: " + heapStats().objectTypeCounts.Response);
+}
diff --git a/test/js/web/fetch/fetch-leak.test.js b/test/js/web/fetch/fetch-leak.test.js
new file mode 100644
index 000000000..fa8b225bd
--- /dev/null
+++ b/test/js/web/fetch/fetch-leak.test.js
@@ -0,0 +1,55 @@
+import { test, expect, describe } from "bun:test";
+import { join } from "node:path";
+import { bunEnv, bunExe } from "harness";
+
+describe("fetch doesn't leak", () => {
+  test("fixture #1", async () => {
+    const body = new Blob(["some body in here!".repeat(100)]);
+    const server = Bun.serve({
+      port: 0,
+
+      fetch(req) {
+        return new Response(body);
+      },
+    });
+
+    const proc = Bun.spawn({
+      env: {
+        ...bunEnv,
+        SERVER: `http://${server.hostname}:${server.port}`,
+      },
+      stderr: "inherit",
+      stdout: "inherit",
+      cmd: [bunExe(), join(import.meta.dir, "fetch-leak-test-fixture.js")],
+    });
+
+    const exitCode = await proc.exited;
+    server.stop(true);
+    expect(exitCode).toBe(0);
+  });
+
+  test("fixture #2", async () => {
+    const body = new Blob(["some body in here!".repeat(100)]);
+    const server = Bun.serve({
+      port: 0,
+
+      fetch(req) {
+        return new Response(body);
+      },
+    });
+
+    const proc = Bun.spawn({
+      env: {
+        ...bunEnv,
+        SERVER: `http://${server.hostname}:${server.port}`,
+      },
+      stderr: "inherit",
+      stdout: "inherit",
+      cmd: [bunExe(), join(import.meta.dir, "fetch-leak-test-fixture-2.js")],
+    });
+
+    const exitCode = await proc.exited;
+    server.stop(true);
+    expect(exitCode).toBe(0);
+  });
+});