src: fix Promise.race() memory leak with deprecated events by kayossouza · Pull Request #60184 · nodejs/node
When Promise.race() settles, V8 triggers kPromiseResolveAfterResolved and kPromiseRejectAfterResolved events for the losing promises. Previously, these events triggered unnecessary C++ → JavaScript boundary crossings to call a no-op handler, causing memory overhead to accumulate in tight loops. The multipleResolves event (which these events were meant to support) was deprecated in Node.js v15 and removed in v17. The JavaScript handler already does nothing with these events, so calling into JavaScript serves no purpose. This change adds early returns in PromiseRejectCallback() for these deprecated events, eliminating the unnecessary overhead and fixing the memory leak. Fixes: #51452
labels
Felipeness added a commit to Felipeness/node that referenced this pull request
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 added a commit to Felipeness/node that referenced this pull request
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
jasnell pushed a commit that referenced this pull request
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>
aduh95 pushed a commit that referenced this pull request
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>
aduh95 pushed a commit that referenced this pull request
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>
araujogui pushed a commit to araujogui/node that referenced this pull request
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. 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: nodejs#51452 Refs: nodejs#60184 Refs: nodejs#61960 PR-URL: nodejs#62336 Refs: nodejs#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>
sxa pushed a commit to sxa/node that referenced this pull request
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. 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: nodejs#51452 Refs: nodejs#60184 Refs: nodejs#61960 PR-URL: nodejs#62336 Refs: nodejs#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> Signed-off-by: Stewart X Addison <sxa@ibm.com>
sxa pushed a commit to sxa/node that referenced this pull request
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. 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: nodejs#51452 Refs: nodejs#60184 Refs: nodejs#61960 PR-URL: nodejs#62336 Refs: nodejs#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> Signed-off-by: Stewart X Addison <sxa@ibm.com>
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