Conversation
…v server did not set it Only the dev server sets the AsyncLocalStorage instance of the bake additions. Outside the dev server the getter returned an empty JSValue. The Rust caller reads an empty value as a thrown exception, and the builtin that wraps a JSX component got the empty value in JavaScript. `Response.redirect()`, `Response.render()` and `new Response(<jsx />)` of "bun:app" crashed the process outside the dev server. The fallbacks in BakeResponse.rs and BakeSSRResponse.ts already handle an instance that is not set. Return `undefined` from the getter so that they run.
|
Status: ready for review. Reproduced on 1.4.3-canary.1+367d939d9 (release) and on a debug build of main a4f1429: cat > redirect.ts <<'EOS'
import { Response } from "bun:app";
console.log(Response.redirect("/login").status);
EOS
bun redirect.ts
#44167 is on top of this PR. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe async-local-storage getter now returns ChangesResponse behavior outside the dev server
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change appears mergeable after normal checks; no actionable risk remains from this review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@robobun wake up!! |
|
@robobun wake up!! |
Problem
Response.redirect(),Response.render()andnew Response(<jsx />)of"bun:app"crash the process outside the dev server. A release build printspanic(main thread): Segmentation fault at address 0x0. A debug build printspanic: Expected an exception to be thrown.BakeAdditionsToGlobalObject::getAsyncLocalStorage(src/jsc/bindings/BakeAdditionsToGlobalObject.h:89) returns an emptyJSValuewhen the dev server did not set the instance.VirtualMachine::get_dev_server_async_local_storagereads that value as a thrown exception, and no exception exists.Fix
undefinedwhen the instance is not set.None.wrapComponentinBakeSSRResponse.tstests it for truth.Response.redirect()now returns a plainResponse.Response.render()throwsResponse.render() is only available in the Bun dev server.test/bake/dev/response-to-bake-response.test.ts(3 new tests, each crashes without the fix). Alsoreact-response.test.tsfor the dev server.Background
"bun:app"exports theResponseclass of Bake. It extends the global class withrender()and with JSX bodies.AsyncLocalStorageinstance on the global object. The three calls read it for the streaming mode.JSValueis the marker for "the host call threw". It is not a JavaScript value.from_js_host_callfrom the Rust caller. The JavaScript caller then still gets the empty value. Both callers read through the getter.Downsides
.textsection: +0 B. The four functions that hold the getter grow by 56 B in total.Notes
Reproduction
Before:
panic(main thread): Segmentation fault at address 0x0, exit code 139 (1.4.3-canary.1+367d939d9). After: prints302.No bundler is necessary for the crash.
bun build --server-componentsandbun build --apprewrite the globalResponseof a server-side file to this class, so built code reaches the same calls.How this was found
No user report exists. A review of #44167, which is on top of this PR and repairs the
bun build --server-componentsoutput, ranResponse.redirect()in the built output.Why the empty value crashes
from_js_host_callhas the contract "the value is empty if and only if an exception was thrown".get_dev_server_async_local_storagereturnsErrfor the empty value. The host function then returns the empty value to JavaScriptCore with no exception set. Debug and ASAN builds assert on that. Release builds continue with the empty value.Only the dev server sets the instance (
src/runtime/bake/DevServer.rs, throughbakeSetAsyncLocalStorage).Not changed here
Response.redirect(url, status)as a render for each status that is not 302. A page that returnsResponse.redirect("/other", 301)gets HTTP 200 with the content of/otherand noLocationheader (also 307 and 308).src/runtime/bake/hmr-runtime-server.tstells a render from a redirect withresp.status !== 302. No PR has this yet.x instanceof Responseis false in a server component whenxcomes fromfetch(),Response.json(),Response.error()orclone(), andBun.inspect(new Response("x"))prints<null />for thebun:appclass. The cause is insrc/jsc/bindings/JSBakeResponse.cpp. No PR has this yet.Measurement
Release builds (
--profile=release) of main a4f1429 and of this change, from one checkout path..textsection: 58,143,477 B in both (llvm-size -A).llvm-nm --print-size):BakeResponseClass__constructForSSR388 -> 408 B,BakeResponseClass__constructRedirect661 -> 673 B,BakeResponseClass__constructRender979 -> 991 B,jsFunctionBakeGetAsyncLocalStorage13 -> 25 B. No other function changes size.Suites run on the debug build
test/bake/dev/response-to-bake-response.test.ts: 8 pass. The same file withBUN_JSC_validateExceptionChecks=1and LeakSanitizer, as the ASAN lane runs it: 8 pass.test/bake/dev/react-response.test.ts: 11 pass.