Racing immediately-resolving Promises leads to memory leak

[cefn]

Version

21.5.0

Platform

Linux penguin 6.1.55-06877-gc83437f2949f #1 SMP PREEMPT_DYNAMIC Thu Dec 14 19:17:39 PST 2023 x86_64 GNU/Linux

Subsystem

async_hooks or async/await

What steps will reproduce the bug?

Run the following code. Failure is quicker and less likely to interfere with system stability if you run with a low heap ceiling like this, but it will fail without...

node --max-old-space-size=16 test/examples/ephemeralPromiseMemoryLeak.js

//ephemeralPromiseMemoryLeak.js

async function promiseValue(value) {
  return value;
}

async function run() {
  for (;;) {
    await Promise.race([promiseValue("foo"), promiseValue("bar")]);
  }
}

run();

An equivalent OOM is created if you substitute Promise.any for Promise.race...

async function promiseValue(value) {
  return value;
}

async function run() {
  for (;;) {
    await Promise.any([promiseValue("foo"), promiseValue("bar")]);
  }
}

run();

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

It always fails.

What is the expected behavior? Why is that the expected behavior?

I would expect it not to accumulate references in memory and fail.

What do you see instead?

Fails with the following error

✗ node test/examples/ephemeralPromiseMemoryLeak.js

<--- Last few GCs --->

[31511:0x6de4330]    39874 ms: Mark-Compact (reduce) 2048.2 (2083.6) -> 2047.3 (2083.9) MB, 1308.67 / 0.00 ms  (average mu = 0.071, current mu = 0.001) allocation failure; scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 0xcc062a node::OOMErrorHandler(char const*, v8::OOMDetails const&) [node]
 2: 0x104eb90 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 3: 0x104ee77 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 4: 0x126e0b5  [node]
 5: 0x126e58e  [node]
 6: 0x12837b6 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GarbageCollectionReason, char const*) [node]
 7: 0x12842d9  [node]
 8: 0x12848e8  [node]
 9: 0x19d4311  [node]
[1]    31511 abort (core dumped)  node test/examples/ephemeralPromiseMemoryLeak.js

Additional information

If the promiseValue call incorporates an explicit scheduling on the event loop, there is no memory leak...

// setImmediateNoLeak.js

function promiseValue(value) {
  return new Promise((resolve) => {
    setImmediate(() => resolve(value));
  });
}

async function run() {
  for (;;) {
    await Promise.race([promiseValue("foo"), promiseValue("bar")]);
  }
}

run();

If the promiseValue call isn't composed via a Promise.race there is no leak...

// noRaceNoLeak.js

async function promiseValue(value) {
  return value;
}

async function run() {
  for (;;) {
    await promiseValue("foo");
    await promiseValue("bar");
  }
}

run();

Maybe obviously, but putting it here for completeness, if you don't use an async await loop, but compose the loop itself with setImmediate there is no leak...

// noLoopNoLeak.js

async function promiseValue(value) {
  return value;
}

async function doRace() {
  await Promise.race([promiseValue("foo"), promiseValue("bar")]);
}

function run() {
  doRace().then(() => setImmediate(run));
}

run();

[aduh95]

/cc @nodejs/v8

[kvakil]

I don't think this is related to V8 per se - it doesn't reproduce using V8's shell, only Node.js. The following hangs-but-doesn't-crash:

$ d8 --max-old-space-size=32 repro.js

Likewise I don't see a memory increase with spidermonkey or javascriptcore.

Running --heapsnapshot-near-heap-limit=1 shows that the references are held by queue here:

node/lib/internal/process/task_queues.js

Line 55 in c25878d

const queue = new FixedQueue();

[cefn]

One workaround prevents the memory leak that, unlike the other workarounds, doesn't assume you can change the composition of the race (e.g. the race might still resolve immediately whether you like it or not). If you ensure resolving calls are interleaved with calls that return to the event loop it eliminates the memory leak...

async function promiseValue(value) {
  return value;
}

function immediateExecutor(resolve) {
  setImmediate(resolve);
}

function promiseImmediate() {
  return new Promise(immediateExecutor);
}

async function run() {
  for (;;) {
    await Promise.race([promiseValue("foo"), promiseValue("bar")]);
    await promiseImmediate();
  }
}

run();

[aduh95]

FWIW there's a promise version of setImmediate: https://nodejs.org/api/timers.html#timerspromisessetimmediatevalue-options.

[cefn]

I'm curious if this is considered a confirmed-bug.

Sounds like you've been able to recreate it and I think it's a bug, and maybe a heisenbug.

Although the failing case is perfectly valid code, I think most of the real-world cases that would create the bug would be poorly-written and resource-wasteful javascript, or some kind of edge-case that I haven't thought of. That's because it must allow itself to enter into a loop repeatedly accessing already-settled values, although combined with Promise.race() the memory overhead might grow very fast since I suspect the resources from jointly raced operations stay in memory too.

If I wrote a tight while loop like this, there would be some kind of visit to the event loop to be more efficient/responsive (enough to implicitly detach the promises for garbage collection). I have been wondering about scenarios where this kind of thing could happen for real to justify actually fixing it.

In my work immediate resolution of functions with a Promise signature is normally down to caching. The promise may be either immediately fulfilled by a (rarely-invalidated) cached value or eventually fulfilled by a cache-refreshing round trip and the caller doesn't know in advance.

However, I think the immediate resolution path may also arise implicitly from already-settled promises where once again the invoking code path can't know in advance they are already settled.

For this reason it could be quite insidious, and could present as a heisenbug, which emerges as a mystifying error from clean-looking, well-tested and long-running code e.g. as soon as you add a caching subroutine your memory blows up and you can't see why. In my view this is not ideal to leave lurking in the node codebase.

[kvakil]

I don't think anyone is disagreeing that this is some sort of bug, although it's not clear to me what the solution is. Someone will need to actually investigate it and figure out how tractable it is.

Anyway, tagged with confirmed-bug.

[cefn]

Repeated races 'polluted' with a resolved promise do seem to retain references in memory to the resources of co-raced promises, even if the co-raced promises are themselves more regular (event loop) promises.

This is illustrated by the relationship between the array length and the number of turns before OOM in the below example...

length	turns
1	4502
10	4255
100	1971
1000	353
10000	41
100000	3

node --max-semi-space-size=8 --max-old-space-size=8 corace.js

// corace.ts

function promiseValue(value) {
  return value;
}

function promiseValueImmediate(value) {
  return new Promise((resolve) => {
    setImmediate(() => {
      resolve(value);
    });
  });
}

let turn = 0;

async function run() {
  for (;;) {
    console.log(turn++);
    await Promise.race([
      promiseValue("foo"),
      promiseValueImmediate(Array.from({ length: 10000 }).map(() => "data")),
    ]);
  }
}

run();

[cefn]

The original repro also hits OOM if you substitute Promise.race with Promise.any...

async function promiseValue(value) {
  return value;
}

async function run() {
  for (;;) {
    await Promise.any([promiseValue("foo"), promiseValue("bar")]);
  }
}

run();

[cefn]

The memory leak issue is eliminated with the example below which has an alternative race implementation.

However if you uncomment the line resolve = null the memory leak comes back.

Thanks to @bnoordhuis #29385 (comment) for the approach.

async function promiseValue(value) {
  return value;
}

function race(promises) {
  return new Promise((resolve, reject) => {
    const tidyResolve = (value) => {
      if (resolve) resolve(value);
      resolve = null;
    };
    promises.forEach((promise) => promise.then(tidyResolve, reject));
  });
}

async function run() {
  for (;;) {
    await race([promiseValue("foo"), promiseValue("bar")]);
  }
}

run();

[rubenpad]

The references are held by the next tick queue, and as the event loop could not start, the promises aren't gc'd. This happens because Promise.race is one of the cases where v8 resolve promises multiple times.

This does not happen in d8 because it ignores those events.

An option to fix it could be calling nextTick only if the multipleResolves has been registered:

function resolveError(type, promise, reason) {
  // We have to wrap this in a next tick. Otherwise the error could be caught by
  // the executed promise.
  if (process.eventNames().includes('multipleResolves')) {
    process.nextTick(() => {
      process.emit('multipleResolves', type, promise, reason);
      multipleResolvesDeprecate();
    });
  }
}