FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: skip JS callback for settled Promise.race losers · nodejs/node@7703f11 · GitHub

/ node Public

Commit 7703f11

Browse files
Felipeness
authored and
aduh95
committed
src: skip JS callback for settled Promise.race losers
When Promise.race() or Promise.any() settles, V8 fires kPromiseResolveAfterResolved / kPromiseRejectAfterResolved for each "losing" promise. The PromiseRejectCallback in node_task_queue.cc was crossing into JS for these events, but since the multipleResolves event reached EOL in v25 (PR #58707), the JS handler does nothing. The unnecessary C++-to-JS boundary crossings accumulate references in a tight loop, causing OOM when using Promise.race() with immediately-resolving promises. Return early in PromiseRejectCallback() for these two events, skipping the JS callback entirely. Also remove the dead case branches and unused constant imports from the JS side. 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. Fixes: #51452 Refs: #60184 Refs: #61960 PR-URL: #62336 Refs: #51452 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent 7e6b77b commit 7703f11

3 files changed

Lines changed: 31 additions & 17 deletions

File tree

‎lib/internal/process/promises.js‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@ const {
1414
promiseRejectEvents: {
1515
kPromiseRejectWithNoHandler,
1616
kPromiseHandlerAddedAfterReject,
17-
kPromiseRejectAfterResolved,
18-
kPromiseResolveAfterResolved,
1917
},
2018
setPromiseRejectCallback,
2119
} = internalBinding('task_queue');
@@ -161,21 +159,15 @@ function promiseRejectHandler(type, promise, reason) {
161159
if (unhandledRejectionsMode === undefined) {
162160
unhandledRejectionsMode = getUnhandledRejectionsMode();
163161
}
162+
// kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are
163+
// filtered out in C++ (src/node_task_queue.cc) and never reach JS.
164164
switch (type) {
165165
case kPromiseRejectWithNoHandler: // 0
166166
unhandledRejection(promise, reason);
167167
break;
168168
case kPromiseHandlerAddedAfterReject: // 1
169169
handledRejection(promise);
170170
break;
171-
case kPromiseRejectAfterResolved: // 2
172-
// Do nothing in this case. Previous we would emit a multipleResolves
173-
// event but that was deprecated then later removed.
174-
break;
175-
case kPromiseResolveAfterResolved: // 3
176-
// Do nothing in this case. Previous we would emit a multipleResolves
177-
// event but that was deprecated then later removed.
178-
break;
179171
}
180172
}
181173

‎src/node_task_queue.cc‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,11 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
5353

5454
Environment* env = Environment::GetCurrent(isolate);
5555

56-
if (env == nullptr || !env->can_call_into_js()) return;
56+
if (env == nullptr || !env->can_call_into_js() ||
57+
event != kPromiseRejectWithNoHandler &&
58+
event != kPromiseHandlerAddedAfterReject) {
59+
return;
60+
}
5761

5862
Local<Function> callback = env->promise_reject_callback();
5963
// The promise is rejected before JS land calls SetPromiseRejectCallback
@@ -77,10 +81,6 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
7781
"rejections",
7882
"unhandled", unhandledRejections,
7983
"handledAfter", rejectionsHandledAfter);
80-
} else if (event == kPromiseResolveAfterResolved) {
81-
value = message.GetValue();
82-
} else if (event == kPromiseRejectAfterResolved) {
83-
value = message.GetValue();
8484
} else {
8585
return;
8686
}
@@ -173,8 +173,6 @@ static void Initialize(Local<Object> target,
173173
Local<Object> events = Object::New(isolate);
174174
NODE_DEFINE_CONSTANT(events, kPromiseRejectWithNoHandler);
175175
NODE_DEFINE_CONSTANT(events, kPromiseHandlerAddedAfterReject);
176-
NODE_DEFINE_CONSTANT(events, kPromiseResolveAfterResolved);
177-
NODE_DEFINE_CONSTANT(events, kPromiseRejectAfterResolved);
178176

179177
target->Set(env->context(),
180178
FIXED_ONE_BYTE_STRING(isolate, "promiseRejectEvents"),

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Flags: --max-old-space-size=64
2+
'use strict';
3+
4+
// Regression test for https://github.com/nodejs/node/issues/51452
5+
// When Promise.race() settles, V8 fires kPromiseResolveAfterResolved /
6+
// kPromiseRejectAfterResolved for each "losing" promise. Before this fix,
7+
// the C++ PromiseRejectCallback crossed into JS for these no-op events,
8+
// accumulating references and causing OOM in tight async loops.
9+
// With --max-old-space-size=64, this test would crash before completing
10+
// if the leak is present.
11+
12+
const common = require('../common');
13+
14+
async function main() {
15+
for (let i = 0; i < 100_000; i++) {
16+
await Promise.race([
17+
Promise.resolve(1),
18+
Promise.resolve(2),
19+
Promise.resolve(3),
20+
]);
21+
}
22+
}
23+
24+
main().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL