'fetch' memory leak introduced in v18.13.0

[jamesdiacono]

Version

v18.13.0

Platform

Darwin 00236c87d925 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

Subsystem

undici

What steps will reproduce the bug?

The memory leak occurs when an AbortSignal is passed to fetch. Running the following program in Node.js v18.13.0 produces the memory leak. The memory leak does not appear in Node.js v18.12.1 or earlier versions.

import http from "http";
const host = "127.0.0.1";
const port = 8844;
const url = "http://" + host + ":" + port;
const server = http.createServer(function on_request(ignore, res) {
    return res.end();
});
server.listen(port, host, function attack() {
    const controller = new AbortController();
    fetch(url, {signal: controller.signal}).then(attack);
});

How often does it reproduce? Is there a required condition?

It is perfectly reproducible on both MacOS and Debian (Linux 5943d9d47ab2 5.10.0-21-cloud-amd64 #1 SMP Debian 5.10.162-1 (2023-01-21) x86_64 GNU/Linux).

What is the expected behavior?

That resources retained during a fetch request be correctly released.

What do you see instead?

A serious memory leak. The following screenshots demonstrate the difference in behaviour between Node.js 18.13.0 and the previous release, 18.12.1.

Additional information

It appears that Undici's use of FinalizationRegistry may be responsible for the leak:

[jamesdiacono]

This issue is duplicate of nodejs/undici#1823. This commit from last month looks like it fixes the problem, but it is yet to make it into a v18 or v19 release.

[stalkerg]

@jamesdiacono seems like they reverted it nodejs/undici#2000, and memory leak back. It's because of this issue nodejs/undici#1926

[jamesdiacono]

Yes, the leak is back in Node.js v19.8.1.

[mcollina]

@ronag this is essentially the reason why I always insist in having tests to avoid regressions. We slipped in a few cases and here we have regressions :(.

[ronag]

This was not a case of we didn't want to have tests... it is difficult to make tests for these kinds of issues.

[ronag]

@jamesdiacono @stalkerg Can you help us with a self-contained repro? Or even better a test we can add to undici.

[mcollina]

This was not a case of we didn't want to have tests... it is difficult to make tests for these kinds of issues.

I know, that's why I LGTM the fix without the test.

[jamesdiacono]

@ronag The repro is at the top of this page.

[ronag]

That's not a self contained repro. The attack method is missing.

[jamesdiacono]

It is not missing, it is recursive.

[stalkerg]

@ronag do you need anything else to solve it? How we can help?

[ronag]

I don't need anything other than time per se. If you want to try to solve it and open a PR that's also welcome.

[jamesdiacono]

Thanks @ronag.

[vampirefrog]

I have stumbled across this same issue in v18.13.0 and that seems to be fixed in v19 but it seems to me that even in v19.9.0 there's a memory leak in fetch() even when you don't use an AbortSignal. I have opened another issue here #51438