Skip to content

Commit 0aa1519

Browse files
committed
src: prevent memory leak from Promise.race with immediate resolves
Do not call JS promise reject callback for kPromiseResolveAfterResolved and kPromiseRejectAfterResolved events. These events are triggered when a promise is resolved/rejected after already being resolved, and notifying JS land causes memory leaks in tight loops with immediately-resolving promises (e.g., Promise.race, Promise.any). While the JS-side handler in lib/internal/process/promises.js already ignores these events (multipleResolves was deprecated and removed), the C++ callback was still being invoked, causing unnecessary async context manipulation and keeping promise object references alive. Fixes: #51452 Signed-off-by: jorge guerrero <grrr.jrg@gmail.com>
1 parent 6b5178f commit 0aa1519

File tree

2 files changed

+110
-4
lines changed

2 files changed

+110
-4
lines changed

src/node_task_queue.cc

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,14 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
7777
"rejections",
7878
"unhandled", unhandledRejections,
7979
"handledAfter", rejectionsHandledAfter);
80-
} else if (event == kPromiseResolveAfterResolved) {
81-
value = message.GetValue();
82-
} else if (event == kPromiseRejectAfterResolved) {
83-
value = message.GetValue();
80+
} else if (event == kPromiseResolveAfterResolved ||
81+
event == kPromiseRejectAfterResolved) {
82+
// Do not notify JS land about these events. These events are triggered
83+
// when a promise is resolved/rejected after already being resolved.
84+
// Notifying JS land would cause memory leaks in tight loops with
85+
// immediately-resolving promises (e.g., Promise.race).
86+
// See: https://github.com/nodejs/node/issues/51452
87+
return;
8488
} else {
8589
return;
8690
}
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// Copyright Node.js contributors. All rights reserved.
2+
'use strict';
3+
4+
// Tests for memory leak in Promise.race with immediately-resolving promises.
5+
// See: https://github.com/nodejs/node/issues/51452
6+
7+
const common = require('../common');
8+
const assert = require('node:assert');
9+
10+
// Test 1: Promise.race with immediately-resolving promises should not leak
11+
{
12+
async function promiseValue(value) {
13+
return value;
14+
}
15+
16+
async function run() {
17+
let count = 0;
18+
const maxIterations = 1000;
19+
20+
for (let i = 0; i < maxIterations; i++) {
21+
await Promise.race([promiseValue("foo"), promiseValue("bar")]);
22+
count++;
23+
}
24+
25+
assert.strictEqual(count, maxIterations);
26+
}
27+
28+
run().then(common.mustCall());
29+
}
30+
31+
// Test 2: Promise.any with immediately-resolving promises should not leak
32+
{
33+
async function promiseValue(value) {
34+
return value;
35+
}
36+
37+
async function run() {
38+
let count = 0;
39+
const maxIterations = 1000;
40+
41+
for (let i = 0; i < maxIterations; i++) {
42+
await Promise.any([promiseValue("foo"), promiseValue("bar")]);
43+
count++;
44+
}
45+
46+
assert.strictEqual(count, maxIterations);
47+
}
48+
49+
run().then(common.mustCall());
50+
}
51+
52+
// Test 3: Mixed immediately-resolving and delayed promises should work correctly
53+
{
54+
async function immediateValue(value) {
55+
return value;
56+
}
57+
58+
function delayedValue(value, delay) {
59+
return new Promise((resolve) => {
60+
setTimeout(() => resolve(value), delay);
61+
});
62+
}
63+
64+
async function run() {
65+
let count = 0;
66+
const maxIterations = 100;
67+
68+
for (let i = 0; i < maxIterations; i++) {
69+
await Promise.race([
70+
immediateValue("immediate"),
71+
delayedValue("delayed", 10)
72+
]);
73+
count++;
74+
}
75+
76+
assert.strictEqual(count, maxIterations);
77+
}
78+
79+
run().then(common.mustCall());
80+
}
81+
82+
// Test 4: Verify that Promise.race still resolves correctly with immediate values
83+
{
84+
async function promiseValue(value) {
85+
return value;
86+
}
87+
88+
Promise.race([promiseValue("first"), promiseValue("second")]).then(common.mustCall((result) => {
89+
assert.strictEqual(result, "first");
90+
}));
91+
}
92+
93+
// Test 5: Verify that Promise.any still resolves correctly with immediate values
94+
{
95+
async function promiseValue(value) {
96+
return value;
97+
}
98+
99+
Promise.any([promiseValue("first"), promiseValue("second")]).then(common.mustCall((result) => {
100+
assert.strictEqual(result, "first");
101+
}));
102+
}

0 commit comments

Comments
 (0)