Repository navigation
Memory leak if unhandled promise rejection mode is set to warn #47158
Description
Version
18.14.0
Platform
Darwin 2.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:04:44 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T8103 arm64
Subsystem
promise
What steps will reproduce the bug?
const util = require("util");
const memwatch = require("@airbnb/node-memwatch");
async function task(ms) {
return new Promise((_, reject) =>
setTimeout(() => {
reject("ummm crashed");
}, ms)
);
}
async function sleep(ms) {
return new Promise((resolve) => setTimeout(resolve, ms));
}
const hd = new memwatch.HeapDiff();
for (let i = 0; i < 1000; i++) {
task(1000);
}
const diff = hd.end();
const hd1 = new memwatch.HeapDiff();
sleep(2000).then(() => {
const diff1 = hd1.end();
console.log(util.inspect(diff, false, null, false /* enable colors */));
console.log(util.inspect(diff1, false, null, false /* enable colors */));
});if you run this program using node --unhandled-rejections=warn temp.js you will see the diff log of memwatch where memory doesn't get free. Now if you change task(1000) to task(1000).catch(e=>{}) the memory will get free.
How often does it reproduce? Is there a required condition?
100% reproducible with node option set to unhandled-rejections=warn.
What is the expected behavior? Why is that the expected behavior?
The memory should get free after printing the warnings on the console.
What do you see instead?
Memory is not getting freed.
Additional information
I ran a clinic js heap profiler and found out that it comes to this function
node/lib/internal/process/promises.js
Line 221 in ad2c3c0
node/lib/internal/process/promises.js
Line 187 in ad2c3c0
The interesting thing is inside processPromiseRejection we do get the promise from maybeUnhandledPromiseMap (
node/lib/internal/process/promises.js
Line 234 in ad2c3c0
Activity
itzmanish commentedon Mar 19, 2023
If I am right about the above memory leak and there is not any PR available for fixing this please let me know I can create a PR to fix this.
bnoordhuis commentedon Mar 19, 2023
This is indeed very likely a duplicate of #43655. I would suggest to close it as such.
maybeUnhandledPromisesis a WeakMap. Entries get collected by the garbage collector eventually but the way V8 handles promises internally keeps them alive for longer than one would intuitively expect. There's no easy fix for that, unfortunately.itzmanish commentedon Mar 20, 2023
But why don't we just remove the promise entry from the
maybeUnhandledPromisesafter completingprocessPromiseRejectionfunction? If we manually delete the promise from the WeakMap after handling the promise rejection there won't be any memory leak.bnoordhuis commentedon Mar 20, 2023
It is. It's done a few lines below the code you linked to.
itzmanish commentedon Mar 20, 2023
Umm. Can you give me a permalink? I am not able to find that.
bnoordhuis commentedon Mar 20, 2023
Sorry, it's a few lines up, not down:
node/lib/internal/process/promises.js
Line 168 in 2984cc3
At the
maybeUnhandledPromises.get(promise)you linked to, I suspect it's not safe yet to remove it from the map.itzmanish commentedon Mar 20, 2023
But heapprofiler doesn't reach that function. see this function
node/lib/internal/process/promises.js
Line 165 in 2984cc3
bnoordhuis commentedon Mar 20, 2023
I don't want to turn this issue into a line-by-line walkthrough of promises.js. I'll explain this one thing but after that you're on your own, okay?
unhandledRejection() is called when a promise without a .catch handler is rejected
handledRejection() is called when a promise without a .catch handler is rejected and then adds a .catch handler
That is, unhandledRejection() and handledRejection() can get called for the same promise, in that order.
That's why unhandledRejection() stores the promise and its associated state in the WeakMap, because handledRejection() may need it later.
itzmanish commentedon Mar 21, 2023
Okay. Thanks for this clarification. Then I think this is already being discussed in #43655.
bnoordhuis commentedon Mar 21, 2023
I'm glad you agree. I'll close this out as a duplicate then.