Skip to content

Commit ec22993

Browse files
committed
fix: improve Promise.race OOM fix placement and cleanup
Move early returns for kPromiseResolveAfterResolved and kPromiseRejectAfterResolved before Number::New and CHECK(!callback), avoiding unnecessary work. Remove dead NODE_DEFINE_CONSTANT exports and fix comment placement in the JS switch statement. Bump test --max-old-space-size from 20 to 64 for safety on instrumented builds.
1 parent 7033537 commit ec22993

File tree

3 files changed

+11
-16
lines changed

3 files changed

+11
-16
lines changed

lib/internal/process/promises.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,16 +159,15 @@ function promiseRejectHandler(type, promise, reason) {
159159
if (unhandledRejectionsMode === undefined) {
160160
unhandledRejectionsMode = getUnhandledRejectionsMode();
161161
}
162+
// kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are
163+
// filtered out in C++ (src/node_task_queue.cc) and never reach JS.
162164
switch (type) {
163165
case kPromiseRejectWithNoHandler: // 0
164166
unhandledRejection(promise, reason);
165167
break;
166168
case kPromiseHandlerAddedAfterReject: // 1
167169
handledRejection(promise);
168170
break;
169-
// kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are
170-
// handled in C++ (src/node_task_queue.cc) by returning early, so they
171-
// never reach this JS callback. No case branches needed.
172171
}
173172
}
174173

src/node_task_queue.cc

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
5555

5656
if (env == nullptr || !env->can_call_into_js()) return;
5757

58+
// multipleResolves was removed in v25 (PR #58707). Skip all work for
59+
// these events to avoid OOM in tight Promise.race() loops (#51452).
60+
if (event == kPromiseResolveAfterResolved ||
61+
event == kPromiseRejectAfterResolved) {
62+
return;
63+
}
64+
5865
Local<Function> callback = env->promise_reject_callback();
5966
// The promise is rejected before JS land calls SetPromiseRejectCallback
6067
// to initializes the promise reject callback during bootstrap.
@@ -77,15 +84,6 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
7784
"rejections",
7885
"unhandled", unhandledRejections,
7986
"handledAfter", rejectionsHandledAfter);
80-
} else if (event == kPromiseResolveAfterResolved) {
81-
// Intentional no-op. The multipleResolves event was EOL'd in v25
82-
// (PR #58707), and the JS handler does nothing for these events.
83-
// Skipping the JS callback avoids accumulating C++-to-JS boundary
84-
// references in tight Promise.race()/Promise.any() loops, which
85-
// previously caused OOM (see #51452).
86-
return;
87-
} else if (event == kPromiseRejectAfterResolved) {
88-
return;
8987
} else {
9088
return;
9189
}
@@ -178,8 +176,6 @@ static void Initialize(Local<Object> target,
178176
Local<Object> events = Object::New(isolate);
179177
NODE_DEFINE_CONSTANT(events, kPromiseRejectWithNoHandler);
180178
NODE_DEFINE_CONSTANT(events, kPromiseHandlerAddedAfterReject);
181-
NODE_DEFINE_CONSTANT(events, kPromiseResolveAfterResolved);
182-
NODE_DEFINE_CONSTANT(events, kPromiseRejectAfterResolved);
183179

184180
target->Set(env->context(),
185181
FIXED_ONE_BYTE_STRING(isolate, "promiseRejectEvents"),

test/parallel/test-promise-race-memory-leak.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
// Flags: --max-old-space-size=20
1+
// Flags: --max-old-space-size=64
22
'use strict';
33

44
// Regression test for https://github.com/nodejs/node/issues/51452
55
// When Promise.race() settles, V8 fires kPromiseResolveAfterResolved /
66
// kPromiseRejectAfterResolved for each "losing" promise. Before this fix,
77
// the C++ PromiseRejectCallback crossed into JS for these no-op events,
88
// accumulating references and causing OOM in tight async loops.
9-
// With --max-old-space-size=20, this test would crash before completing
9+
// With --max-old-space-size=64, this test would crash before completing
1010
// if the leak is present.
1111

1212
const common = require('../common');

0 commit comments

Comments
 (0)