src: skip JS callback for settled Promise.race losers#62336
src: skip JS callback for settled Promise.race losers#62336Felipeness wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
c++
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 nodejs#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. Fixes: nodejs#51452 Refs: nodejs#60184 Refs: nodejs#61960
Felipeness
force-pushed
the
fix/promise-race-oom
branch
from
debf239 to
7033537
Compare
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.
|
I have an open CL for a patch to V8 which would effectively remove these events on the V8 side, incidentally. |
|
That's great context, thanks for sharing! Took a look at your CL, really cool to see the root cause being addressed on the V8 side. I think both changes complement each other well. This Node-side fix is a small early return for events that have been no-ops since Would you mind linking your CL in #51452 as well? Would be great context for anyone following the issue. |
| unhandledRejectionsMode = getUnhandledRejectionsMode(); | ||
| } | ||
| // kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are | ||
| // filtered out in C++ (src/node_task_queue.cc) and never reach JS. |
There was a problem hiding this comment.
Is there a harm from keeping the cases and panicking if they’re hit?
There was a problem hiding this comment.
kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are no longer even defined on the JS side after this patch.
|
Hey @ljharb @benjamingr 👋 Thank you both for the reviews and the thoughtful discussion — really appreciate the time you put into it. Just wanted to check in: is there anything else you'd like me to address or improve before this moves forward? Happy to make any adjustments. If everything looks good, would one of you mind triggering CI whenever you get a chance? No rush at all. Thanks again! |
| if (event == kPromiseResolveAfterResolved || | ||
| event == kPromiseRejectAfterResolved) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nit: these can just be folded up into the early return if check immediately above and the comment likely isn't all that necessary
|
Thanks @jasnell for the approval and the nit, and @gurgunday for the review. I'll fold the early return as suggested and push the fix shortly. Took a small break after some friction on #62340 around Windows build difficulties, but coming back to wrap this one up That's all for today, folks! |
7 participants
Summary
Fixes #51452
When
Promise.race()settles, V8 fireskPromiseResolveAfterResolved/kPromiseRejectAfterResolvedfor each "losing" promise. Node'sPromiseRejectCallbackinsrc/node_task_queue.ccwas crossing into JS for these events, but since themultipleResolvesevent reached EOL in Node v25 (PR #58707), the JS handler does nothing — it justbreaks.The unnecessary C++-to-JS boundary crossings accumulate references in a tight loop, causing OOM when using
Promise.race()with immediately-resolving promises.Fix
Return early in
PromiseRejectCallback()forkPromiseResolveAfterResolvedandkPromiseRejectAfterResolved, skipping the JS callback entirely. Also remove the deadcasebranches and unused constant imports from the JS side.Previous attempts
This PR resubmits the fix with a regression test.
Test
Added
test/parallel/test-promise-race-memory-leak.jsthat runs 100k iterations ofPromise.race()with immediately-resolving promises under--max-old-space-size=20. Before the fix, this OOMs; after the fix, it completes normally.Refs: #51452
Refs: #60184
Refs: #61960