src: skip JS callback for settled Promise.race losers#62336
Open
Felipeness wants to merge 2 commits intonodejs:mainfrom
Open
src: skip JS callback for settled Promise.race losers#62336Felipeness wants to merge 2 commits intonodejs:mainfrom
Felipeness 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.
Contributor
|
I have an open CL for a patch to V8 which would effectively remove these events on the V8 side, incidentally. |
Author
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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