oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
71.67k stars 2.54k forks source link

Add finalizer when the ReadableStreamReader finalizes while the body is being read #10763

Open Jarred-Sumner opened 1 month ago

Jarred-Sumner commented 1 month ago

In the fetch() client, if we've started to read an in-flight response body but do not finish reading it and neglect to call cancel on the ReadableStream, then we do not close the stream and the response body leaks. Node.js' fetch has the same bug.

Client:

const server = process.argv.at(-1);
const fmt = new Intl.NumberFormat();
let total = 0;
const batch = 50;
const delay = 0;
while (true) {
  const array = new Array(batch);
  for (let i = 0; i < batch; i++) {
    // ✅ .then(() => {})
    // ✅ .getReader()
    // ❌  const reader = r.body.getReader(); reader.read()
    // ❌  const reader = r.body.getReader(); reader.read().then(() => reader.releaseLock());
    // ✅  const body = r.body; const reader = body.getReader(); reader.read().then(() => {reader.releaseLock(); body.cancel();});
    array[i] = fetch(server).then((r) =>  r.body.getReader().read());
  }
  await Promise.all(array);
  await new Promise((r) => setTimeout(r, delay));
  const current = (process.memoryUsage.rss() / 1024 / 1024) | 0;
  console.log(
    "[" +
      Math.trunc(performance.now())
        .toString(10)
        .padStart(6, "0") +
      "ms]",
    "RSS",
    (process.memoryUsage.rss() / 1024 / 1024) | 0,
    "MB after",
    fmt.format((total += batch))
  );
}

Server:

const blob = new Blob([require("crypto").randomFillSync( new Uint8Array(1024 * 512))]);

Bun.serve({
  fetch(req) {
    return new Response(blob);
  },
  port: process.argv.at(-1),
});

Something involving the ReadableStream reader needs to either be a WeakRef in JS or a JSC::Weak in Zig/C++.

Jarred-Sumner commented 2 weeks ago

@cirospaciari did you fix this? I think you might've

cirospaciari commented 2 weeks ago

Its not fixed yet, but is a easier fix now, today fetch has a strong ref with the fetch task if we start streaming and will buffer the full body waiting, we can switch for a weak ref here too but I remember that I did not fix it because it was finalizing too early and I did not handle pending reads correctly. In short we need to keep alive until pending reads (probably ref counting here) are finish and after this deinit and ignore the rest.