jsc: drain rejected-promise list in O(n) instead of O(n^2) - #32554
Merged
Conversation
`GlobalObject::handleRejectedPromises()` looped on `WriteBarrierList::takeFirst()`, which acquires `cellLock()` and calls `Vector::removeAt(0)` (memmove of the remaining elements) once per promise — O(n^2) total work for n queued rejections. Replace it with a single `drainTo()` that moves the whole list into a `MarkedArgumentBuffer` under one lock, then iterates linearly. An outer do-while preserves the original semantics for rejections raised from inside an `unhandledRejection` handler. Adds an early return for the common empty case (this runs on every event-loop tick). `WriteBarrierList::takeFirst()` is now unused and removed. This matches the move-out-then-iterate pattern JSC's `VM::didExhaustMicrotaskQueue` and WebCore's `RejectedPromiseTracker` already use. | pending rejections | before | after | |-------------------:|---------:|--------:| | 1,000 | 0.09 ms | 0.06 ms | | 5,000 | 1.17 ms | 0.25 ms | | 10,000 | 4.35 ms | 0.45 ms | | 20,000 | 19.13 ms | 0.88 ms |
Collaborator
|
Updated 2:23 PM PT - Jun 25th, 2026
✅ @robobun, your commit 8aec644ae933530f359960a32928a8145972020c passed in 🧪 To try this PR locally: bunx bun-pr 32554That installs a local version of the PR into your bun-32554 --bun |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesUnhandled Rejection Drain Refactor
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- promiseRejectionTracker(Handle) now also scans the in-flight tail of the drained snapshot, so a handler that .catch()es a later still-pending rejection no longer fires a spurious 'rejectionHandled' (it never got 'unhandledRejection'). Test extended to cover this. - WriteBarrierList::moveTo() folded into drainTo() (only caller switched). - Trim comments that referenced the removed takeFirst() implementation.
handleRejectedPromises() can re-enter (a handler that ticks the event loop). The previous flat pointer/index pair was overwritten by the inner call and unconditionally nulled on its return, leaving the outer loop without its in-flight state. Link the in-flight snapshots as a stack-allocated singly-linked list and restore via SetForScope, and have promiseRejectionTracker(Handle) walk every level so a .catch() from an inner handler on a promise still pending in an outer frame is also recognised.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/ZigGlobalObject.cpp (1)
3302-3304:⚠️ Potential issue | 🟠 MajorReplace RELEASE_ASSERT with graceful error handling for promise rejection queue overflow.
MarkedArgumentBufferis an argument buffer with overflow semantics; draining an unboundedWriteBarrierList<JSPromise>into it can turn user-triggered resource exhaustion into a crash. Although the event loop normally drains rejections frequently, a pathological script that creates promises faster than the event loop processes them will overflow the buffer. Per coding guidelines, user-reachable failures must be recoverable errors, never panics — drain in bounded chunks or switch to a GC-rooted container without argument-buffer overflow semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/ZigGlobalObject.cpp` around lines 3302 - 3304, The RELEASE_ASSERT checking for overflow in the MarkedArgumentBuffer after draining m_aboutToBeNotifiedRejectedPromises converts user-triggered resource exhaustion into a crash, violating the requirement that user-reachable failures must be recoverable. Replace the RELEASE_ASSERT(!promises.hasOverflowed()) check with graceful error handling by either draining m_aboutToBeNotifiedRejectedPromises in bounded chunks into the MarkedArgumentBuffer instead of all at once, or by switching to a GC-rooted container without overflow semantics that can hold an unbounded number of rejected promises without panic-on-overflow behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3302-3304: The RELEASE_ASSERT checking for overflow in the
MarkedArgumentBuffer after draining m_aboutToBeNotifiedRejectedPromises converts
user-triggered resource exhaustion into a crash, violating the requirement that
user-reachable failures must be recoverable. Replace the
RELEASE_ASSERT(!promises.hasOverflowed()) check with graceful error handling by
either draining m_aboutToBeNotifiedRejectedPromises in bounded chunks into the
MarkedArgumentBuffer instead of all at once, or by switching to a GC-rooted
container without overflow semantics that can hold an unbounded number of
rejected promises without panic-on-overflow behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 216af42c-dd2f-4432-97fe-2b29f1a9a8b3
📒 Files selected for processing (2)
src/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.h
| inflight.index = i + 1; | ||
|
|
||
| Bun__handleRejectedPromise(this, promise); | ||
| if (auto ex = scope.exception()) { |
Collaborator
There was a problem hiding this comment.
What if its a termination exception? tryClearException will return false and what we should do, in that case, is return out of this function.
Collaborator
There was a problem hiding this comment.
Done in 8aec644: checks vm.isTerminationException(ex) before tryClearException() and returns early, matching drainMicrotasks() in this file and VM::didExhaustMicrotaskQueue() upstream. The stack-scoped SetForScope restores m_rejectedPromisesBeingProcessed on the early return.
robobun
added a commit
that referenced
this pull request
Jul 2, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
This was referenced Jul 5, 2026
robobun
added a commit
that referenced
this pull request
Jul 14, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
robobun
added a commit
that referenced
this pull request
Jul 18, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
robobun
added a commit
that referenced
this pull request
Jul 18, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
robobun
added a commit
that referenced
this pull request
Jul 18, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
robobun
added a commit
that referenced
this pull request
Jul 18, 2026
…the installed window, harden tests - promiseRejectionTracker(Reject) now goes through AsyncContextFrame::withAsyncContextIfNeeded instead of open-coding it, dropping a redundant tracking-enabled gate and a dead null check. - unhandled_rejection()'s microtask drains and the auto-GC that follows the dispatch no longer run with the rejected promise's async context installed. Node drains outside its exchange window, and the propagation machinery assumes the ambient context is undefined during a top-level drain. - Tests: make each restore clause individually load-bearing (a contextless rejection drained from inside a context; a drain that must restore the slot afterwards), pin the rejection-time semantic against the creation-time one, cover the frame-wrapped Handle path in both unwrap sites (same-tick catch, plus the #32554 regression test parametrized with AsyncLocalStorage), and assert --unhandled-rejections=strict keeps the context for uncaughtException but not for the drain that follows.
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.
What
GlobalObject::handleRejectedPromises()looped onWriteBarrierList::takeFirst(), which doescellLock()+Vector::removeAt(0)(memmove) per element — O(n²) for n queued rejections.This replaces it with
drainTo()(move the whole list into aMarkedArgumentBufferunder one lock) + linear iteration. An outerdo-whilekeeps the original behaviour for rejections raised from inside anunhandledRejectionhandler. Also adds anisEmpty()early return for the common no-rejections-pending tick.takeFirst()is now dead and removed.This is the same move-out-then-iterate pattern JSC's
VM::didExhaustMicrotaskQueueand WebCore'sRejectedPromiseTrackeralready use.Benchmark
Release build, aarch64-darwin.
for (i<N) Promise.reject(i)then drive one macrotask:Before scales ~quadratically (×2 → ×4), after scales linearly (×2 → ×2). Normal microtask throughput (
await/.then/queueMicrotask) and memory usage are unchanged.Test
Added a subprocess test in
test/js/node/process/process.test.jsthat rejects 1000 promises in one tick and asserts:.catch()ed before the checkpoint is not delivered