Consolidate AsyncLocalStorage save/restore with an RAII AsyncContextSwapScope - #301
Conversation
Introduces a small stack-only RAII helper that encapsulates the "read tuple, save current, swap in, run, restore" pattern currently open-coded at every InternalMicrotask case that runs user code under a captured AsyncLocalStorage context. The helper is header-only and ALWAYS_INLINE so codegen is unchanged; the fast path (asyncContext undefined) remains a single branch. Static helpers cover the adjacent snapshot-side patterns (reading the current context and wrapping/unwrapping it in an InternalFieldTuple alongside a user context). No callers yet; the mechanical replacements follow in separate commits.
…PromisePrototype) Mechanical replacement of the snapshot-side blocks with the static helpers introduced in the previous commit: - performPromiseThen / resolveWithInternalMicrotaskForAsyncAwait / JSPromisePrototype finally(): wrapWithCurrent(vm, globalObject, userContext) - resolvePromise thenable-job queuing: current(globalObject) performPromiseThenWithContext keeps its original "wrap whenever userContext is defined" rule (using current() for the asyncContext read) rather than wrapWithCurrent: callers may pass their own InternalFieldTuple as userContext (ReadableStream async iterator does), and PromiseReactionJob would otherwise mistake it for a [_, asyncContext] wrapper. No semantic change.
….cpp Mechanical replacement of the per-InternalMicrotask-case open-coded "read tuple, save current, swap in, run, restore" blocks with AsyncContextSwapScope (plus unwrapContextTuple where the async context arrives as field 1 of an InternalFieldTuple). AsyncFunctionResume's error / completed-generator branches keep their existing "restore before promise->reject / promise->resolve" ordering via restoreEarly(), as does BunPerformMicrotaskJob's "restore before error reporting" step. Everywhere else the scope naturally restores at return. PromiseReactionJob previously had no restore on the two RETURN_IF_EXCEPTION paths after promiseOrCapability.get(reject / resolve) throws; the RAII scope now restores on those paths as well. All other call sites are byte-for-byte equivalent when the scope is a no-op (asyncContext empty or undefined), which is the common path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds ChangesAsync context propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@Source/JavaScriptCore/runtime/JSMicrotask.cpp`:
- Around line 1616-1620: Move the AsyncContextSwapScope initialization in the
promise resolution job so it is established before the
promiseSpeciesWatchpointIsValid check and any call to
promiseResolveThenableJobFastSlow. Preserve the existing arguments[2] context
source and ensure both fast and slow paths execute within the captured context.
- Around line 1887-1890: Update the Async-from-Sync continuation in the code
around AsyncContextSwapScope::unwrapContextTuple to retain and apply the tuple’s
field 1 as the async context before invoking
asyncFromSyncIteratorContinueOrDone. Ensure the captured context from await is
installed for the continuation instead of being discarded, while preserving the
existing promise and completion handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecca483d-9740-47d8-acc6-375c0fefc1cc
📒 Files selected for processing (5)
Source/JavaScriptCore/CMakeLists.txtSource/JavaScriptCore/runtime/AsyncContextSwapScope.hSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSPromise.cppSource/JavaScriptCore/runtime/JSPromisePrototype.cpp
| if (!promiseSpeciesWatchpointIsValid(vm, promise)) [[unlikely]] | ||
| RELEASE_AND_RETURN(scope, promiseResolveThenableJobFastSlow(globalObject, promise, promiseToResolve)); | ||
|
|
||
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Set up async context for promise resolution | ||
| InternalFieldTuple* asyncContextData = nullptr; | ||
| JSValue restoreAsyncContext; | ||
| if (!asyncContext.isUndefined()) { | ||
| asyncContextData = globalObject->m_asyncContextData.get(); | ||
| if (asyncContextData) { | ||
| restoreAsyncContext = asyncContextData->getInternalField(0); | ||
| asyncContextData->putInternalField(vm, 0, asyncContext); | ||
| } | ||
| } | ||
| AsyncContextSwapScope asyncContextScope(vm, globalObject, arguments[2]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Establish the context scope before entering the slow fallback.
When the species watchpoint is invalid, Line 1617 calls promiseResolveThenableJobFastSlow before arguments[2] is installed. Observable species/then operations therefore run outside the context captured when this job was queued.
Proposed fix
+#if USE(BUN_JSC_ADDITIONS)
+ AsyncContextSwapScope asyncContextScope(vm, globalObject, arguments[2]);
+#endif
+
if (!promiseSpeciesWatchpointIsValid(vm, promise)) [[unlikely]]
RELEASE_AND_RETURN(scope, promiseResolveThenableJobFastSlow(globalObject, promise, promiseToResolve));
-
-#if USE(BUN_JSC_ADDITIONS)
- AsyncContextSwapScope asyncContextScope(vm, globalObject, arguments[2]);
-#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!promiseSpeciesWatchpointIsValid(vm, promise)) [[unlikely]] | |
| RELEASE_AND_RETURN(scope, promiseResolveThenableJobFastSlow(globalObject, promise, promiseToResolve)); | |
| #if USE(BUN_JSC_ADDITIONS) | |
| // Set up async context for promise resolution | |
| InternalFieldTuple* asyncContextData = nullptr; | |
| JSValue restoreAsyncContext; | |
| if (!asyncContext.isUndefined()) { | |
| asyncContextData = globalObject->m_asyncContextData.get(); | |
| if (asyncContextData) { | |
| restoreAsyncContext = asyncContextData->getInternalField(0); | |
| asyncContextData->putInternalField(vm, 0, asyncContext); | |
| } | |
| } | |
| AsyncContextSwapScope asyncContextScope(vm, globalObject, arguments[2]); | |
| `#if` USE(BUN_JSC_ADDITIONS) | |
| AsyncContextSwapScope asyncContextScope(vm, globalObject, arguments[2]); | |
| `#endif` | |
| if (!promiseSpeciesWatchpointIsValid(vm, promise)) [[unlikely]] | |
| RELEASE_AND_RETURN(scope, promiseResolveThenableJobFastSlow(globalObject, promise, promiseToResolve)); |
🤖 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 `@Source/JavaScriptCore/runtime/JSMicrotask.cpp` around lines 1616 - 1620, Move
the AsyncContextSwapScope initialization in the promise resolution job so it is
established before the promiseSpeciesWatchpointIsValid check and any call to
promiseResolveThenableJobFastSlow. Preserve the existing arguments[2] context
source and ensure both fast and slow paths execute within the captured context.
| JSValue contextArg = arguments[2]; | ||
| if (auto* tuple = dynamicDowncast<InternalFieldTuple>(contextArg)) | ||
| contextArg = tuple->getInternalField(0); | ||
| AsyncContextSwapScope::unwrapContextTuple(contextArg); | ||
| auto* promise = uncheckedDowncast<JSPromise>(asObject(contextArg)->getDirect(vm, vm.propertyNames->builtinNames().promisePrivateName())); | ||
| RELEASE_AND_RETURN(scope, asyncFromSyncIteratorContinueOrDone(promise->realm(), vm, promise, contextArg, arguments[1], static_cast<JSPromise::Status>(payload), task == InternalMicrotask::AsyncFromSyncIteratorDone)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the unwrapped async context instead of discarding it.
Line 1888 strips the tuple but ignores field 1, so these Async-from-Sync continuations execute under the ambient context rather than the context captured at await.
Proposed fix
`#if` USE(BUN_JSC_ADDITIONS)
JSValue contextArg = arguments[2];
- AsyncContextSwapScope::unwrapContextTuple(contextArg);
+ AsyncContextSwapScope asyncContextScope(vm, globalObject, AsyncContextSwapScope::unwrapContextTuple(contextArg));
auto* promise = uncheckedDowncast<JSPromise>(asObject(contextArg)->getDirect(vm, vm.propertyNames->builtinNames().promisePrivateName()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JSValue contextArg = arguments[2]; | |
| if (auto* tuple = dynamicDowncast<InternalFieldTuple>(contextArg)) | |
| contextArg = tuple->getInternalField(0); | |
| AsyncContextSwapScope::unwrapContextTuple(contextArg); | |
| auto* promise = uncheckedDowncast<JSPromise>(asObject(contextArg)->getDirect(vm, vm.propertyNames->builtinNames().promisePrivateName())); | |
| RELEASE_AND_RETURN(scope, asyncFromSyncIteratorContinueOrDone(promise->realm(), vm, promise, contextArg, arguments[1], static_cast<JSPromise::Status>(payload), task == InternalMicrotask::AsyncFromSyncIteratorDone)); | |
| JSValue contextArg = arguments[2]; | |
| AsyncContextSwapScope asyncContextScope(vm, globalObject, AsyncContextSwapScope::unwrapContextTuple(contextArg)); | |
| auto* promise = uncheckedDowncast<JSPromise>(asObject(contextArg)->getDirect(vm, vm.propertyNames->builtinNames().promisePrivateName())); | |
| RELEASE_AND_RETURN(scope, asyncFromSyncIteratorContinueOrDone(promise->realm(), vm, promise, contextArg, arguments[1], static_cast<JSPromise::Status>(payload), task == InternalMicrotask::AsyncFromSyncIteratorDone)); |
🤖 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 `@Source/JavaScriptCore/runtime/JSMicrotask.cpp` around lines 1887 - 1890,
Update the Async-from-Sync continuation in the code around
AsyncContextSwapScope::unwrapContextTuple to retain and apply the tuple’s field
1 as the async context before invoking asyncFromSyncIteratorContinueOrDone.
Ensure the captured context from await is installed for the continuation instead
of being discarded, while preserving the existing promise and completion
handling.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~450-line refactor across the promise/microtask hot path with a few intentional behavior changes, so it's worth a human look.
What was reviewed:
- Traced each of the ~13
runInternalMicrotaskcases: RAII destructor ordering matches the old explicit-restore ordering (includingrestoreEarly()sites and the unified non-Bun#elsebranches). unwrapContextTuplevs. old open-coded extraction inPromiseReactionJob: empty/null/non-cellarguments[3]still routes to the 1-argcallMicrotaskbranch.performPromiseThenWithContext: dropping the outerm_asyncContextDatanull-guard now wraps a defineduserContexteven when tracking is disabled — this is a fix (protects a user-suppliedInternalFieldTuplefrom being mis-unwrapped downstream), consistent with the PR's stated rationale, but not called out separately.- Confirmed the 4-arg
InternalFieldTuple::createoverload exists.
Extended reasoning...
Overview
Introduces a header-only RAII helper AsyncContextSwapScope (new file, 122 lines) and mechanically replaces ~13 open-coded save/swap/restore blocks in JSMicrotask.cpp plus 4 capture-side blocks in JSPromise.cpp / JSPromisePrototype.cpp. Net -328 lines. All code is guarded by USE(BUN_JSC_ADDITIONS), but several #if/#else branches are unified so the non-Bun path is also reshaped (verified equivalent — RELEASE_AND_RETURN on a void call ≡ scope.release(); call(); return;).
Security risks
None identified. This is internal AsyncLocalStorage context plumbing; no user-controlled input parsing, no auth/crypto, no boundary crossing. The RAII helper is WTF_FORBID_HEAP_ALLOCATION / WTF_MAKE_NONCOPYABLE and holds a raw InternalFieldTuple* only for the stack lifetime of a single microtask dispatch.
Level of scrutiny
High. runInternalMicrotask / performPromiseThen / resolvePromise are on the hot path for every await, .then, and microtask in Bun. A dropped restore or a mis-ordered destructor here corrupts AsyncLocalStorage for the rest of the event loop tick, and a stray allocation regresses promise throughput. The PR is a mechanical refactor and the author ran the full async_hooks / Node parallel suite under debug+ASAN, but the three intentional behavior changes (two RETURN_IF_EXCEPTION paths in PromiseReactionJob now restore; performPromiseThenWithContext now wraps whenever userContext is defined even if m_asyncContextData is null) deserve human sign-off.
Other factors
- I walked each converted case and confirmed the RAII destructor fires at the same point the old explicit
putInternalField(vm, 0, restoreAsyncContext)did, including the tworestoreEarly()call sites (AsyncFunctionResume reject/resolve, BunPerformMicrotaskJob error reporting). PromiseReactionJob: old code normalized empty/undefined/nullcontextArgtouserContext = jsUndefined(); new code leaves it as-is but the downstreamuserContext.isEmpty() || userContext.isUndefinedOrNull()branch treats all three identically, so no observable change.unwrapContextTupleguardscontextArg.isEmpty()beforedynamicDowncast, matching the old explicit check.performPromiseThenWithContext: the dropped outerif (m_asyncContextData)guard means a defineduserContextis now always tuple-wrapped even when async-context tracking was never enabled. This is the fix the PR description alludes to (prevents a caller-suppliedInternalFieldTuple— e.g. ReadableStream async iterator — from being mis-unwrapped inPromiseReactionJob), but it's a behavior change on a path the listed test suites may not exercise with tracking disabled.- Verified
InternalFieldTuple::create(vm, structure, a, b)overload exists (used bywrapWithCurrentand the rewrittenperformPromiseThenWithContext).
Given the criticality of the touched code and the presence of deliberate behavior changes, deferring rather than auto-approving.
|
@robobun fix the errors |
There was a problem hiding this comment.
I didn't find any bugs, but this refactor touches the core promise/microtask dispatch path across ~13 InternalMicrotask cases, so it warrants a human look before merging.
What was reviewed:
- Verified each replaced block in
JSMicrotask.cpppreserves the prior swap/restore ordering (includingrestoreEarly()at the two sites that restored before the tail). - Confirmed the two CodeRabbit flags (
PromiseResolveThenableJobFastslow-path ordering,AsyncFromSyncIteratordiscarding field 1) match pre-PR behavior — not regressions introduced here. - Checked
unwrapContextTuple's empty-JSValue guard againstPromiseReactionJob's possibly-emptyarguments[3]. - Noted
performPromiseThenWithContextnow wraps whenuserContextis defined even ifm_asyncContextDatais null (previously it did not) — consistent with the stated intent, but a real behavior delta.
Extended reasoning...
Overview
Introduces a header-only RAII helper AsyncContextSwapScope and replaces ~13 open-coded "read tuple / save / swap / run / restore" blocks across JSMicrotask.cpp, JSPromise.cpp, and JSPromisePrototype.cpp. Net ~328 lines removed. All changes are guarded by USE(BUN_JSC_ADDITIONS).
Security risks
None identified. This is internal engine plumbing for AsyncLocalStorage context propagation; no parsing, auth, or externally-controlled data flow is involved.
Level of scrutiny
High. This is the hot path for every promise reaction, async function resume, and async generator step in Bun's runtime. A dropped or mis-ordered restore leaks async context across unrelated microtasks, and such bugs are notoriously hard to reproduce. While the transformation is mechanical in intent, each of the 13 sites has slightly different control flow (early returns, RETURN_IF_EXCEPTION, restoreEarly() ordering), and the PR itself documents two intentional behavior changes plus a subtle third in performPromiseThenWithContext (wrapping now happens when m_asyncContextData is unset but userContext is defined).
Other factors
- The two CodeRabbit "Major" findings both describe pre-existing behavior that the PR faithfully preserves (the old code also swapped after the species-watchpoint check, and also discarded field 1 in
AsyncFromSyncIterator); they are not regressions, but the maintainer's follow-up "fix the errors" comment has no visible resolution yet. - The four
AsyncGenerator*cases dropped their separate non-Bun#elsebranch in favor of a unified body; I verified the resulting non-Bun path is equivalent (theRELEASE_AND_RETURNbecamescope.release(); ...; return;). - The author reports Bun's async_hooks and vendored Node async-local-storage test suites pass under debug+ASAN, which is good coverage for the happy paths but may not exercise the exception-during-
.get(reject)edge or the null-m_asyncContextDatadelta.
Given the breadth, the criticality of the code path, and the unresolved maintainer comment, deferring to human review.
Preview Builds
|
…tion, Dockerfile.windows) JSMicrotask.cpp: adopt the AsyncContextSwapScope RAII helper (#301) in place of the manual m_asyncContextData save/restore blocks, keeping upstream's microtaskCallCache threading and the asyncFunctionGeneratorBodyCall extraction. The AsyncGeneratorDriverResume enqueue sites and dispatch case are converted to wrapWithCurrent / unwrapContextTuple to match the siblings.
2 participants
Summary
Bun's
AsyncLocalStoragethreads its context through the promise/microtask internals viaJSGlobalObject::m_asyncContextData(anInternalFieldTuple, field 0 holds the current context). The restore side of this, inruntime/JSMicrotask.cpp, had the same 8-line "read tuple, save current, swap in, run, restore" block copy-pasted across ~13InternalMicrotaskcases (147 mentions ofasyncContextin the file), with the same pattern repeated on the capture side inJSPromise.cpp/JSPromisePrototype.cpp. Every upstream change that reshapes a microtask case is a merge conflict and a chance to silently drop a restore.This introduces a small header-only RAII helper,
AsyncContextSwapScope, and mechanically replaces every open-coded block with it.The helper
runtime/AsyncContextSwapScope.h(guarded byUSE(BUN_JSC_ADDITIONS)):(VM&, JSGlobalObject*, JSValue asyncContext): swapsasyncContextintom_asyncContextDatafield 0 and remembers the previous value; a no-op whenasyncContextis empty or undefined (single branch on the common path).restoreEarly(): restores the previous value.restoreEarly()is for the two call sites whose existing ordering restores before the tail of the case (AsyncFunctionResume'spromise->reject/resolve, BunPerformMicrotaskJob's error reporting).unwrapContextTuple(JSValue&): if the argument is anInternalFieldTuple[userContext, asyncContext], replaces it with field 0 and returns field 1; otherwise leaves it unchanged and returnsjsUndefined(). Tolerates an emptyJSValue(thedynamicDowncast<T>(JSValue)overload is not empty-safe by itself, and PromiseReactionJob'sarguments[3]can be empty).current(JSGlobalObject*)/wrapWithCurrent(VM&, JSGlobalObject*, JSValue userContext): capture-side helpers for reading the current context and wrapping it alongside a user context.All methods are
ALWAYS_INLINE; the class isWTF_FORBID_HEAP_ALLOCATION/WTF_MAKE_NONCOPYABLE.Behavior
Strictly behavior-preserving, with two deliberate exceptions noted in the commits:
PromiseReactionJobhad twoRETURN_IF_EXCEPTIONpaths (afterpromiseOrCapability.get(reject)/.get(resolve)throws) that returned without restoring; the RAII scope now restores there as well.performPromiseThenWithContextis not switched towrapWithCurrent: it must keep wrapping wheneveruserContextis defined even if no async context is active, because callers may pass their ownInternalFieldTupleasuserContext(ReadableStream's async iterator does) andPromiseReactionJobwould otherwise unwrap it as[_, asyncContext]. It now reads the current context viaAsyncContextSwapScope::current()but keeps its original wrapping condition.The zero-cost-when-unused properties are preserved: slim / inline reactions still apply when the context is empty, no new fields on reactions, no new allocations on any path that did not already allocate, no changes to existing
[[likely]]/ early-out fast paths.Diff shape
Net: ~328 lines removed across the three .cpp files;
asyncContext*mentions inJSMicrotask.cppdrop from 147 to 21.Verification
Built Bun against this branch via
bun run build:local(debug + ASAN,ASSERT_ENABLED). All of the following pass identically to the prebuilt-WebKit baseline:test/js/node/async_hooks/(AsyncLocalStorage.test.ts, AsyncLocalStorage-tracking.test.ts, async-local-storage-thenable.test.ts, async-context/): 111 pass / 0 fail / 3 todotest-async-local-storage-{bind,contexts,deep-stack,enter-with,exit-does-not-leak,http-multiclients,snapshot}.js,test-http2-async-local-storage.js,test-stream-finished-async-local-storage.js: all exit 0test/js/web/streams/streams.test.js: 157 pass / 2 fail (same two pre-existing timeouts as baseline)Commits
Split for review:
AsyncContextSwapScope(no callers)JSPromise.cpp/JSPromisePrototype.cppJSMicrotask.cpp