src: skip JS callback for settled Promise.race losers - #62336
Conversation
Sorry, something went wrong.
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
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. |
Sorry, something went wrong.
|
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 multipleResolves was removed in v25, so it can ship now and help users already hitting the OOM. Once your V8 change lands and rolls into Node, the early return just becomes unreachable and can be cleaned up easily. Would you mind linking your CL in #51452 as well? Would be great context for anyone following the issue. |
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
kPromiseRejectAfterResolved and kPromiseResolveAfterResolved are no longer even defined on the JS side after this patch.
Sorry, something went wrong.
There was a problem hiding this comment.
They also never worked
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks @bakkot @benjamingr, that covers it!
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
| 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
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
ping @jasnell or whoever, can we land this? |
Sorry, something went wrong.
Sorry, something went wrong.
Co-authored-by: Kevin Gibbons <bakkot@gmail.com>
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>
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>
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>
|
Cannot be backported to Node.js 24 because DEP0160 is not EOL there |
Sorry, something went wrong.
Summary
Fixes #51452
When Promise.race() settles, V8 fires kPromiseResolveAfterResolved / kPromiseRejectAfterResolved for each "losing" promise. Node's PromiseRejectCallback in src/node_task_queue.cc was crossing into JS for these events, but since the multipleResolves event reached EOL in Node v25 (PR #58707), the JS handler does nothing — it just breaks.
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() for kPromiseResolveAfterResolved and kPromiseRejectAfterResolved, skipping the JS callback entirely. Also remove the dead case branches 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.js that runs 100k iterations of Promise.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