1761401 - Promise.race() with unsettled promise holds reaction record forever

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P3)

Description

[cobrafast]

Attachment: 20220325-035728.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:98.0) Gecko/20100101 Firefox/98.0

Steps to reproduce:

The following exemplary code results in a fast growing unending memory leak.

var x = new Promise(() => {});
(async () => {
while(true) {
await Promise.race([x, Promise.resolve()]);
}
})().catch(console.error.bind(console));

A friend of mine stumbled upon this writing video streaming webapp, where he is using Promise.race() in a loop to decide between a recurring fast completing promise (<1s) and a slow overarching promise that might take several hours to complete. I took the time to understand the problem and his code looks like a common and valid use case to me. Any memory attached to those promises seems to be pinned indefinitely too and memory leaks much faster than above example.

Firefox 98.0.2 and 100.0a1 (2022-03-24) are affected.

This seems to be very similar to this old problem in Node: https://github.com/nodejs/node/issues/17469

It might be loosely related to this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1540101

Actual results:

Firefox used up all available memory over the course of a minute or two.

Expected results:

Promises should have been properly discarded or unsubscribed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Performance' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Tooru Fujisawa [:arai]]

This is not memory leak, as already pointed out in the issues linked above.

what's happening here is the following:

with Promise.race([x, ...]):

• a new PromiseObject = P1 is created
• a new PromiseReactionRecord = R1 is created, to resolve P1 once x is resolved
• R1 is appended to the x's reactions array

Then, even if P1 is resolved by other promise passed to Promise.race, x still holds R1 in its reactions array, until x resolves.
But x never resolves, and the reactions array holds appended reactions forever.
So, essentially, the above code is creating too long array of PromiseReactionRecord, and those PromiseReactionRecords are shown in the attached screenshot.

This matches to the spec:
https://tc39.es/ecma262/#sec-performpromisethen

27.2.5.4.1 PerformPromiseThen ( promise, onFulfilled, onRejected [ , resultCapability ] )
...
  9. If promise.[[PromiseState]] is pending, then
    a. Append fulfillReaction as the last element of the List that is promise.[[PromiseFulfillReactions]].
    b. Append rejectReaction as the last element of the List that is promise.[[PromiseRejectReactions]].

The [[PromiseFulfillReactions]] and [[PromiseRejectReactions]] are Lists that's created when the promise is created (= reactions array in our impl), and discarded only when the promise is resolved (either fulfilled or rejected)
https://tc39.es/ecma262/#sec-promise-executor

27.2.3.1 Promise ( executor )
...
  5. Set promise.[[PromiseFulfillReactions]] to a new empty List.
  6. Set promise.[[PromiseRejectReactions]] to a new empty List.

https://tc39.es/ecma262/#sec-fulfillpromise

27.2.1.4 FulfillPromise ( promise, value )
...
  4. Set promise.[[PromiseFulfillReactions]] to undefined.
  5. Set promise.[[PromiseRejectReactions]] to undefined.

https://tc39.es/ecma262/#sec-rejectpromise

27.2.1.7 RejectPromise ( promise, reason )
...
  4. Set promise.[[PromiseFulfillReactions]] to undefined.
  5. Set promise.[[PromiseRejectReactions]] to undefined.

So, the question here is, does the minimal testcase really represent the original issue?
Why x never resolves and there's Promise.race with x ?

Also, if the minimal testcase really represents the original issue and this case can frequently happen in wild,
it would be nice to address it in the spec, instead of some hack in each implementation.

Comment 3

[cobrafast]

That v8 bug is interesting, especially since webkit based browsers (Edge and Chrome I checked) do not seem to exhibit this leaking behaviour currently.

The concrete use case was reading from a long running fetch() response, which can fire several times per second, and also waiting for some other interaction that might cause the fetch to become obsolete, like the user clicking "abort" which they might never do. Think await Promise.race([reader.read(), longTask]);. In this case any data from the fetch reader was also pinned indefinitely.

This could be written along the lines of while (!longTaskCompleted) { await reader.read(); }, which could have undesired side effects if the read() gets unresponsive for some reason (e.g. unstable internet connection); the user would not be able to abort the process right away.

I know that fetch specifically can also be canceled with an AbortController but other APIs or libraries might not have that available and it's hard to predict where it may be needed. Promise.race() inside a loop seems like an intuitive universal solution.

I would also agree that the spec was correctly followed. Seems like use cases like these were an oversight and perhaps an update is in order.

Comment 4

[Tooru Fujisawa [:arai]]

I confirmed the high memory usage and then tab crash (except on Safari) on the following, with the minimal testcase above:

• Chrome 99.0.4844.84 (arm64) on macOS
• Edge 99.0.1150.52 (arm64) on macOS
• Safari 15.4 (17613.1.17.1.6) on macOS (confirmed 38GB memory usage)

So, if the original issue doesn't happen on them, it would mean that the issue is in somewhere different than what the minimal testcase covers.

Then, about the actual use case, the logic there means that the "abort" promise is going to affect all promises generated by Promise.race.
So fundamentally that doesn't work unless items inside the list of affected promises are removed once the affected promise is resolved.
In the current spec, that doesn't happen, and you'd need to do the similar thing manually by something like the following:

// A set of reject functions for abortPromise that doesn't not yet lose the race.
var rejectSet = new Set();

function abort() {
  for (var resolve of rejectSet) {
    resolve();
  }
}

...

// Create a temporary promise, that's rejected if `abort` is called.
var reject;
var abortPromise = new Promise((_, r) => { reject = r; });
rejectSet.add(reject);

var P = Promise.race([abortPromise, reader.read()]);

P.finally(() => {
  // Remove the reject function so that there's no reference alive.
  rejectSet.delete(reject);
});

await P;

Then, indeed AbortController should be the solution for fetch + abort,
and in case some other asynchronous Web API lacks such feature, it would be better addressing the issue by introducing similar thing to that API itself, so that there's no need for race and that should be simpler.
If you have specific use case other than fetch that doesn't fit, let me know.