fix: fix wrong stream canceled up after cloning (v7)

[snyamathi]

Pretty much the same as #4414 but for v7

This relates to...

• use FinalizationRegistry for cloned response body #3458
• Cloned request asynchronously changes to bodyUsed #4150

Rationale

The request/stream registered with the finalization registry is mixed up between the clone and the original request.

Changes

This PR just changes which of the two tee'd streams is associated

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[Uzlopak]

seems wrong.

[snyamathi]

This is all out of date - ignore this comment

@Uzlopak this is re-adding the same code from #3458 which you had reviewed.

There were some modifications before it was reverted in #4320 to address the issue #4150

I think it is still of value to clean up the response body if a request is cloned, especially since the docs say:

The Fetch Standard allows users to skip consuming the response body by relying on garbage collection to release connection resources.

Although they do go on to say

it is important to always either consume or cancel the response body anyway.

This being a clone vs a fetch makes it unclear if the body will be garbage collected or not, and some frameworks like Next.JS clone every request by default to provide deduping of feches from a single inbound request

[snyamathi]

This fails on main

✖ clone body garbage collection should not affect original res (6.698336ms)
  AssertionError [ERR_ASSERTION]: Got unwanted exception: Body has already been consumed

[snyamathi]

res has never had its body read, but ends up being canceled in the finalization registry b/c we register the wrong stream in lib/web/fetch/response.js

[snyamathi]

When the clone is garbage collected, the original res body stream should not be canceled

[mcollina]

Why did you revert #4320?

[snyamathi]

Sorry, I ended up not needing to revert it. I'll update the PR description, but I've squashed that change out.

The only change now is updating which tee'd stream is put into the finalization registry

[snyamathi]

https://github.com/snyamathi/nextjs-mem-leak/blob/main/index.mjs Here's a minimal reproduction of the issue. I ended up finding this while tracing down a memory leak in NextJS

/**
 * This is a minimal reproduction of an issue in
 * https://github.com/nodejs/undici/pull/3458
 *
 * When cloning a response, the wrong stream is
 * registered with undici's finalization registry
 * causing the original stream to be reclaimed, not
 * the cloned stream.
 */

import { setImmediate } from "node:timers/promises";

const { promise, resolve } = Promise.withResolvers();
const registry = new FinalizationRegistry(resolve);

const doFetch = async () => {
  const res = await fetch(
    "https://microsoftedge.github.io/Demos/json-dummy-data/512KB-min.json"
  );

  // Clone res and consume its body
  const clone = res.clone();
  await clone.json();

  // Use finalization registry to know when the *clone* has been reclaimed
  registry.register(clone, {});

  return res;
};

const res = await doFetch();

global.gc();
await promise;
await setImmediate();

// TypeError: Response.clone: Body has already been consumed.
res.clone();

[tsctx]

You've caused a regression of the memory leak issue in the cloned response.

> node --expose-gc ./test/fetch/fire-and-forget.js
✔ test finalizer cloned request (103.7647ms)
RSS 81 MB after 50 fetch() requests
RSS 84 MB after 100 fetch() requests
...
RSS 536 MB after 4900 fetch() requests
RSS 537 MB after 4950 fetch() requests
RSS 541 MB after 5000 fetch() requests
✔ does not need the body to be consumed to continue (10846.5894ms)
ℹ tests 2
ℹ suites 0
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 10973.2636

[Uzlopak]

I actually wonder what actually happens when you tee. Maybe the issue is, that we expect that both streams get consumed? And our expectations are wrong?

[mcollina]

Maybe the issue is, that we expect that both streams get consumed?

No, each stream could be consumed independently.

[Uzlopak]

@mcollina
Sure, both streams could be consumed independently. But if it isnt, how do we know that they are done and can be gc'ed?

[mcollina]

The finalizationregistry/weakref business should make sure of it.

[tsctx]

I wrote a better solution in #4419 's PR.

[Uzlopak]

I merged the other PR. Please verify and if you like please backport for v6

[snyamathi]

@Uzlopak I've updated #4414 to backport the changes for v6