Node: Memory leak on Promise.all with async/await

[marian2js]

• Version: 14.5 / 12.18
• Platform: macOS Catalina / MacBook Pro 2019 (Darwin 19.5.0 Darwin Kernel Version 19.5.0: Tue May 26 20:41:44 PDT 2020; root:xnu-6153.121.2~2/RELEASE_X86_64 x86_64)

What steps will reproduce the bug?

(async function () {
  const a = new Array(10).fill(0)
  for (let i = 0; i < 1e8; i++) {
    await Promise.all(a.map(() => Promise.resolve()))
    if (i % 1e5 === 0) {
      console.log(Math.round(process.memoryUsage().heapUsed / 1024 / 1024), 'MB')
    }
  }
})()

Output:

454 MB
78 MB
140 MB
230 MB
152 MB
216 MB
250 MB
303 MB
367 MB
431 MB
496 MB
710 MB
774 MB
838 MB
891 MB
956 MB
1020 MB
1073 MB
1137 MB
1201 MB
1266 MB
RangeError: Value undefined out of range for undefined options property undefined
    at Map.set (<anonymous>)
    at AsyncHook.init (domain.js:68:15)
    at PromiseWrap.emitInitNative (internal/async_hooks.js:154:43)
    at Promise.then (<anonymous>)
    at Function.all (<anonymous>)
    at repl:4:19
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

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

The issue is always reproducible, I tried it in node 14.5 and 12.18.

What is the expected behavior?

Memory usage shouldn't increase since we are waiting for all promises to be resolved before moving to the next iteration.

[bnoordhuis]

I can't reproduce with master - memory remains stable - so this is likely a bug that's been fixed but not released yet.

cc @nodejs/async_hooks - does any of you know what commit / PR contains the fix?

[Flarna]

I can't reproduce this with 12.18.2 either. But I tested on windows and linux only - not mac.

[Flarna]

I can reproduce something like this (even on master) if I prepend following code:

const { createHook } = require("async_hooks");

const m = new Map();
createHook({
  init(id) {
    m.set(id, "val");
  },
  destroy(id) {
    m.delete(id);
  }
}).enable();

Reason is most likely that destroy hook gets never called as it is scheduled via SetImmediate but the more or less endless running promise look is not allowing the process to get into the next tick.

see https://github.com/nodejs/node/blob/9a0aaa610706bfe7aeb2234b085cdcb912403c90/src/async_wrap.cc#L765

[bnoordhuis]

I thought that error message looked familiar! This is a duplicate of #33896, see the example in https://github.com/nodejs/node/issues/33896#issuecomment-645841361.

cc @addaleax

[Flarna]

I think using isolate->EnqueueMicrotask() could help here as it has the same priority then promises so it should trigger the destroy hook inbetween cleanup.

I did a quick check and it seems to work in this usecase (some tests are failing which needs to be checked).
@addaleax Do you expect any negative impact by moving to EnqueueMicrotask? Maybe performance as it get less bulking?

[puzpuzpuz]

The original script probably has to be run in REPL. REPL creates a domain which in its turn installs a hook. I'm AFK and can't verify this, but that might explain the above messages.

[Crusader4Christ]

node
Welcome to Node.js v12.16.1.
Type ".help" for more information.

(async function () {
... const a = new Array(10).fill(0)
... for (let i = 0; i < 1e8; i++) {
..... await Promise.all(a.map(() => Promise.resolve()))
..... if (i % 1e5 === 0) {
....... console.log(Math.round(process.memoryUsage().heapUsed / 1024 / 1024), 'MB')
....... }
..... }
... })()
Promise { }
3 MB
208 MB
457 MB
380 MB
709 MB
875 MB
1051 MB
1217 MB
RangeError: Value undefined out of range for undefined options property undefined

[mayrbenjamin92]

Hmm for me this looks like your sample code is heavily blocking the event-loop. Hmm.

[ronkorving]

I'm with @mayrbenjamin92 here. The code seems to inflate memory by design. It synchronously creates a ton of promises. How could it not inflate memory?

[marian2js]

@ronkorving The code is not synchronous because there is the await in the 4th line. We are waiting for the promises to be resolved before going to the next iteration in the loop and creating new promises.
This issue can be closed because it's a duplicated of #33896.

[ronkorving]

@marian2js My bad, I don't know what I was thinking (or seeing).