Repository navigation
Memory leak on Promise.all with async/await #34328
Description
Activity
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?
I can't reproduce this with 12.18.2 either. But I tested on windows and linux only - not mac.
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
SetImmediatebut the more or less endless running promise look is not allowing the process to get into the next tick.see
Line 765 in 9a0aaa6
Reacted by Anna Henningsen and Andrei PechkurovI thought that error message looked familiar! This is a duplicate of #33896, see the example in #33896 (comment).
cc @addaleax
Reacted by Anna Henningsen and Andrei PechkurovI 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 toEnqueueMicrotask? Maybe performance as it get less bulking?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.
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 undefinedHmm for me this looks like your sample code is heavily blocking the event-loop. Hmm.
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?
@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.@marian2js My bad, I don't know what I was thinking (or seeing).
- added 2 commits that reference this issue
on Jul 30, 2020 - added a commit that references this issue
on Aug 5, 2020 - added 2 commits that reference this issue
on Sep 22, 2020
What steps will reproduce the bug?
Output:
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.