Memory leak when aborting fetch requests

[jamesdiacono]

trafficstars

Bug Description

A memory leak occurs when frequently aborting fetch requests.

Reproducible By

Run the following code in Node.js.

import undici from "undici";

(function attack() {
    const controller = new AbortController();
    undici.fetch(
        "http://idonotexist:9999",
        {signal: controller.signal}
    ).catch(attack);
    controller.abort();
}());

In less than a minute, I see memory usage of the Node.js process climb to several gigabytes.

Expected Behavior

Memory usage to remain stable, as is the case when you comment out the controller.abort(); statement.

Environment

$ uname -a
Darwin MacBook-Pro.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64 arm Darwin
$ node -v
v20.4.0

[mcollina]

looks like we are not clearing up resources in case of an abort

[metcoder95]

Does the same happens for request or others, or only within fetch?

I’ll try to look at it later Today

[KhafraDev]

cc @ronag

[jamesdiacono]

@metcoder95 I'm sorry, I don't know. I have never used request.

[metcoder95]

No worries! But I was referring to undici.request 😅

[Uzlopak]

Yes, as @KhafraDev mentioned it is basically https://github.com/nodejs/node/issues/43655

It is because of pendingUnhandledRejections keeping references on the promises, so that node can remove it it later from the WeakMap.

https://github.com/nodejs/node/blob/64c6d97463c29bade4d6081683dab2cd7cda298d/lib/internal/process/promises.js#L161

[Uzlopak]

@KhafraDev

Can or should we even implement a solution in undici?

[KhafraDev]

Can - yes (?), as your PR did that.

Should - yes, we can always revert the change once it's fixed upstream. We've implemented plenty of workarounds for bugs in node core in the past.

[Uzlopak]

This is a bug, which needs to be fixed in node core, as it is a "bug" in the event loop. This bug is basically introduced by the logic which handles the unhandledPromiseRejected warning. Basically this line results in this issue:

https://github.com/nodejs/node/blob/544cfc5ef151bca8d625fbccc581200a77b00bc0/lib/internal/process/promises.js#L161

So because we keep a reference to the promise for "at least one tick" v8 can not garbage collect the promise. So what happens is that before the event loop can go to the "next" iteration, it gets another promise to handle and starts again processing the same iteration of the event loop as processPromiseRejections returns true.

https://github.com/nodejs/node/blob/544cfc5ef151bca8d625fbccc581200a77b00bc0/lib/internal/process/task_queues.js#L96

[mcollina]

The previous promises should be collectable, no? Why is it staying alive? If that's the case you should be able to reproduce it just with an abortcontroller or a self-rejecting promise chain.

[Uzlopak]

Exactly. You dont need fetch. You can use the example in the corresponding node issue https://github.com/nodejs/node/issues/43655

[mcollina]

Then, we close this.