Cloned request asynchronously changes to bodyUsed

[CreativeTechGuy]

trafficstars

Bug Description

If a request is cloned and the body of the cloned request is read (in a single chained instruction), after a delay the body of the original request will become "used".

Reproducible By

After some amount of time, the output will change to true for bodyUsed on the original request. Depending on the machine is how long this will take (usually within several seconds).

Note: It's intentional that the request will error so it'll keep retrying.

const request = new Request("http://localhost:1234/abc", {
    method: "POST",
    body: JSON.stringify({ a: 1 })
});

await retry(request);

async function retry(req) {
    console.log(await request.clone().text());
    for (let i = 0; i < 1000; i++) {
        console.log(i, req.bodyUsed);
        await new Promise((resolve) => setTimeout(resolve, 10));
    }
    try {
        await fetch(req.clone());
    } catch (e) {
        console.log(e);
        return await retry(req);
    }
}

The example below, which should be identical, doesn't have the issue:

const request = new Request("http://localhost:1234/abc", {
    method: "POST",
    body: JSON.stringify({ a: 1 })
});

await retry(request);

async function retry(req) {
    const clonedRequest = request.clone();
    console.log(await clonedRequest.text());
    for (let i = 0; i < 1000; i++) {
        console.log(i, req.bodyUsed);
        await new Promise((resolve) => setTimeout(resolve, 10));
    }
    try {
        await fetch(req.clone());
    } catch (e) {
        console.log(e);
        return await retry(req);
    }
}

Expected Behavior

A cloned request wouldn't affect the original, and the code style shouldn't affect the behavior.

Logs & Screenshots

N/A

Environment

Reproducible on Node 23.11.0 and 22.14.0 (haven't tried others but I assume other versions have a similar problem).

Additional context

This can be reproduced on multiple machines with multiple OSes with multiple Node versions. The amount of iterations seems "random" per machine, but once you see how long it takes on your machine to magically change, it'll be pretty consistently reproducible in the same amount of time each time you re-run.

[CreativeTechGuy]

I've followed up by testing it on the following Node.js versions:

• 20.19.0 - Broken
• 19.8.1 - Cannot reproduce
• 18.20.8 - Cannot reproduce

[mcollina]

This is a tricky area because it's full of potential memory leaks. In other words, we must manually track each cloned request in a FinalizationRegistry to make things work according to the actual spec. My guess is that for whatever reason the "original" request got tangled in this and it's consumed, even if it shouldn't.

[metcoder95]

My bare guess seems that it roots to the way you are cloning the request before the actual loop:

console.log(await request.clone().text());
// vs
const cloned = request.clone()
console.log(await request.text());

Haven't do a full research tho. cc: @KhafraDev

[tsctx]

https://github.com/nodejs/undici/blob/9dd11b8c61c95efd5459f375a196a117184230fa/lib/web/fetch/body.js#L297

This seems to be the cause.

The difference with that is that it's happening because a reference isn't held.

[mcollina]

@tsctx I think the underlinig bug is that we are passing through cloneBody and adding to that finalizationregistry also for requests, which we probably shouldn't.

https://github.com/nodejs/undici/pull/3458 @KhafraDev you added that here. The PR is titling only "Response", and the test covers only the response cloning. My guess is that was a mistake. Can you confirm?

[code-forger]

This bug can commonly occur when using Promise.all over api calls of different speeds.

E.G.

async function wrappedFetch(url) {
  const response = fetch(url);
  const text = await response.clone().text();
  // Some code handling text, perhaps looking for body sent error codes
  return response;
}

async function getMultipleData() {
  const [ slowRes, fastRes ] = await Promise.all([
    wrappedFetch('slow.url.com'),
    wrappedFetch('fast.url.com'),
  ]);

  return [ await slowRes.json(), await fastRes.json() ];
}

The reason this exposes the bug is because as Creative Tech Guy says, the body is marked as used 'After some amount of time <...> Depending on the machine is how long this will take (usually within several seconds).'

Since here we clone and read the text within the wrapped function, that will happen much earlier for the fast api than the slow api, giving time for the bug to manifest before the .json() is finally called on the fast api response.

I'm not sure this helps track down or fix this bug, but maybe makes this issue easier to find for people searching ' TypeError: Body is unusable: Body has already been read + Promise.all '

[code-forger]

FYI, Removing the chained res.clone().text() did not solve this for us.

Our race condition still exists, and one of the two bodies still almost always throws the 'Body has already been read' error.

So to us, it appears that

const text = await response.clone().text();

and

const clone = response.clone();
const text = await clone.text():

Both exhibit this bug.

[tsctx]

@code-forger That appears to be a slightly different issue (as this is a request, not a response). I'd respectfully suggest creating a new issue for it. Ideally, it would be highly beneficial if we could reproduce it without requiring an external server.