Memory leak if unhandled promise rejection mode is set to warn

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

function processPromiseRejections() {

and then it moved to this function (

node/lib/internal/process/promises.js

Line 187 in ad2c3c0

function emitUnhandledRejectionWarning(uid, reason) {

) as expected because of the option I have set in node options.

The interesting thing is inside processPromiseRejection we do get the promise from maybeUnhandledPromiseMap (

node/lib/internal/process/promises.js

Line 234 in ad2c3c0

const promiseInfo = maybeUnhandledPromises.get(promise);

) but we don't delete after printing the warning, which I think keeps increasing and thus creating a memory leak.

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.

This is indeed very likely a duplicate of #43655. I would suggest to close it as such.

maybeUnhandledPromises is 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.

But why don't we just remove the promise entry from the maybeUnhandledPromises after completing processPromiseRejection function? If we manually delete the promise from the WeakMap after handling the promise rejection there won't be any memory leak.

It is. It's done a few lines below the code you linked to.

Umm. Can you give me a permalink? I am not able to find that.

Sorry, it's a few lines up, not down:

node/lib/internal/process/promises.js

Line 168 in 2984cc3

maybeUnhandledPromises.delete(promise);

At the maybeUnhandledPromises.get(promise) you linked to, I suspect it's not safe yet to remove it from the map.

But heapprofiler doesn't reach that function. see this function

node/lib/internal/process/promises.js

Line 165 in 2984cc3

function handledRejection(promise) {

never got executed.

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.

Okay. Thanks for this clarification. Then I think this is already being discussed in #43655.

I'm glad you agree. I'll close this out as a duplicate then.

Navigation Menu

Repository navigation

Memory leak if unhandled promise rejection mode is set to warn #47158

Version

Platform

Subsystem

What steps will reproduce the bug?

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

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

What do you see instead?

Additional information

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Memory leak if unhandled promise rejection mode is set to warn #47158

Description

Version

Platform

Subsystem

What steps will reproduce the bug?

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

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

What do you see instead?

Additional information

Activity

itzmanish commented on Mar 19, 2023

bnoordhuis commented on Mar 19, 2023

itzmanish commented on Mar 20, 2023

bnoordhuis commented on Mar 20, 2023

itzmanish commented on Mar 20, 2023

bnoordhuis commented on Mar 20, 2023

itzmanish commented on Mar 20, 2023

bnoordhuis commented on Mar 20, 2023

itzmanish commented on Mar 21, 2023

bnoordhuis commented on Mar 21, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions