Conversation
Since v1.4.1 an error that Bun creates natively has no async frames when the rejected promise is awaited through finally(): an fs.promises call followed by .finally(cb) rejects with no stack at all, and finally() further up the await chain cuts the stack after the first frame. The walk over the pending reactions found the promise finally() returned in the reaction's cell slot. oven-sh/WebKit#552 (#41190) put the async context in that slot. The reaction's context is a JSSlimPromiseReaction record whose promise() is that promise, so follow the record, as JSC's own reaction walk in AbstractModuleRecord.cpp does. Read the context once per promise through JSPromise::asyncStackTraceContext(), which covers the inline reaction and the head of the list.
|
Status: ready for review. The diff is green in CI. One unrelated test is red. Repro: Tests: CI (build 116888): 178 jobs passed and the two darwin x64 test jobs still run. The one red job is the debian x64-asan shard with Order: this has to land before the WebKit bump that carries oven-sh/WebKit#268. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe async stack tracer now follows ChangesAsync stack tracing
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change safely follows promises returned by finally handlers, and the added tests cover the stated rejection paths. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This branch has not been deployed
2 participants
Problem
finally().await fs.promises.readFile(missing).finally(() => {})rejects withe.stack === undefined. Withfinally()further up the await chain, the stack stops after the first frame. Bun 1.4.0 and Node print every frame.collectAsyncStackFramesFromPromise(src/jsc/bindings/AsyncStackTrace.cpp) walks the pending reactions of the rejected promise. It found the promisefinally()returned in the reaction's cell slot. Carry the AsyncLocalStorage frame in promise reactions and microtask arguments instead of an InternalFieldTuple WebKit#552 (carried by AsyncLocalStorage: drop enterWith() frames at the event-loop boundary, stop allocating per await/then #41190) put the async context there, so the walk stops.Fix
JSSlimPromiseReactionrecord, the walk followsrecord->promise(), the promisefinally()returned. JSC's own reaction walk (importPromiseGatesAsyncDependency) does the same.JSPromise::asyncStackTraceContext(), which covers the inline reaction and the head of the list.finally()phase to the same layout. Two of the new tests guard that phase.test/js/node/fs/promises.test.js, 8 new tests, 6 fail on 1.4.3-canary. Alsobun-file.test.tsandstreams.test.js.Background
Bun.file, streams) has no JS call stack.Bun__attachAsyncStackFromPromisebuilds one from the async functions that await the promise.await, the context is the async function's generator.finally()fast path registers an internal reaction whose context is a small record. The record holds the promisefinally()returned.Notes
Versions. The same probe (
await readFile(missing).finally(() => {})): 1.4.0 printsat asyncframes, 1.4.1 and 1.4.3-canaryc6b7fcb5bgivee.stack === undefined, this branch prints the frames, with and without anAsyncLocalStoragecontext. oven-sh/WebKit#552 changedpromiseProtoFuncFinallyto registerPromiseFinallyReactionJobwith no cell.Tests. The two tests named "the promise the callback returned rejects" pass on main. They are guards for oven-sh/WebKit#268. With that change linked in and without this fix, that phase loses its frames inside
AsyncLocalStorage.run(). With this fix all 8 tests pass on it, also on its published preview build (autobuild-preview-pr-268-2cb7572a).Not covered, on purpose. This PR follows
finally()only. The walk still reads only the newest reaction of a promise, so a handler attached after the awaiting reaction (const done = p.finally(cb); p.catch(log); await done, or the same with a plainawait) hides it. That is the same on 1.4.0. The walk also still stops atPromise.all/allSettled/any/race(the file header says so), at custom thenables and at Promise subclasses. Errors thrown from JS get their async stack from JSC'sInterpreter::getAsyncStackTrace, not from this walk (#23760). Native sites that do not callreject_with_async_stack, and fetch aborts and timeouts, get no async stack with or without this PR.Follow-up.
JSPromise::forEachPendingReaction(in the fork since oven-sh/WebKit#552) reports every reaction with its task kind. A port of this walk to it removes the layout knowledge that broke here, and these lines with it.Other open PRs on this file. #38074 changes
Bun__attachAsyncStackFromPromiseand does not overlap. #35685 rewrites the two lines where the walk reads the context. Whichever of the two lands second needs a small rebase in that loop.Self-review. The review asked for the regression framing, the mid-chain test, the reaction-list test, assertions on the whole leading frame list and one read of the context per promise. All are in.
[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file