Add Bun streaming server renderer#25597
Conversation
|
Hi @colinhacks! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Comparing: f31005d...9ed1e91 Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gnoff
left a comment
There was a problem hiding this comment.
An exciting PR! I think the build options need to be considered some more. Also there are runtime specific tests for node/browser to ensure key apis work in every environment. Can you port those tests to a Bun specific implementation?
FYI you can also test against built artifacts from this PR: https://react-builds.vercel.app/api/prs/25597/packages/react-dom You can also test against a specific commit. https://react-builds.vercel.app/ explains this in more detail |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thanks for the comments @gnoff! I've addressed your reviews and comments. Regarding tests: I could port new ReadableStream({ type: 'direct' })Since all tests are currently executed with |
|
@colinhacks Ah, that's right, i forgot that our browser test is kinda cheating b/c it is running in a mock web standards environment with Jest. You're right it doesn't make sense to add the tests unless we can actually run it in the propert runtime. I'm going to look into this b/c we ought to do tests in all the runtimes really. If you're so included and want to hack on it as well to get something working for Bun too that would be cool. I'll follow up in a few days when I know if this is something I'm acutally going to prioritize right now-ish |
gnoff
left a comment
There was a problem hiding this comment.
Thanks! the other changes look good. I left a few more comments and also see the CLA still needs to be completed for this to be mergable
91b409f to
d7078c6
Compare
@gnoff What's the next step for the CLA? I signed it yesterday + added me & @colinhacks under "Schedule A" |
Performance numbersFor a "hello world" component served over http on an M1 in Bun v0.2.2
Overall, this makes SSR'ing React sent over HTTP/HTTPS in Bun up to 3.5x faster, without changing the public API interface. When using There's nothing React-specific about the optimizations in this PR. The performance win comes from Bun's
|
gnoff
left a comment
There was a problem hiding this comment.
I think this is good now. I had one more nit re: package.json
@sebmarkbage I haven't made progress on changing our testing strategy meaningfully for server runtimes so no imminent solution to actually running these tests in CI in a Bun runtime. Is solving the testing issue a blocker to merging this in your mind?
|
We can probably land without tests for now. |
|
Thanks @gnoff for your help with this! Just addressed that final review, I think this is good to go 👍 |
Add support for Bun server renderer
Add support for Bun server renderer
Summary: Three problems popped up during the sync: - react/react@07f46ecf2 breaks breaks tests - react/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test. - https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in react/react#26064 This sync includes the following changes: - **[17f6912](react/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([#25752](react/react#25752)) //<Samuel Susla>// - **[f101c2d](react/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([#25775](react/react#25775)) //<Jan Kassens>// - **[420f0b7](react/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([#25774](react/react#25774)) //<Jan Kassens>// - **[3ba7add](react/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([#25338](react/react#25338)) //<Sebastian Silbermann>// - **[fa11bd6](react/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([#25437](react/react#25437)) //<mofeiZ>// - **[e982254](react/react@e98225485 )**: Add ref cleanup function ([#25686](react/react#25686)) //<Samuel Susla>// - **[15557fa](react/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([#25713](react/react#25713)) //<Josh Story>// - **[8a23def](react/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([#25711](react/react#25711)) //<Tianyu Yao>// - **[2655c93](react/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([#25645](react/react#25645)) //<Jimmy Lai>// - **[c08d8b8](react/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([#25708](react/react#25708)) //<Tianyu Yao>// - **[56ffca8](react/react@56ffca8b9 )**: Add Bun streaming server renderer ([#25597](react/react#25597)) //<Colin McDonnell>// - **[f31005d](react/react@f31005d6a )**: Add SyncHydrationLane ([#25698](react/react#25698)) //<Tianyu Yao>// - **[f284d9f](react/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>// - **[6b4c031](react/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>// - **[33e3d28](react/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>// - **[4387d75](react/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>// - **[5eb78d0](react/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>// - **[4a2d86b](react/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>// - **[9dfbd9f](react/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>// - **[44c4e6f](react/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([#25695](react/react#25695)) //<Andrew Clark>// - **[7b17f7b](react/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([#25699](react/react#25699)) //<Sebastian Markbåge>// - **[6fb8133](react/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([#25383](react/react#25383)) //<Sebastian Silbermann>// - **[07f46ec](react/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([#25697](react/react#25697)) //<Sebastian Markbåge>// - **[d65b88d](react/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([#25696](react/react#25696)) //<Sebastian Markbåge>// - **[c343f80](react/react@c343f8025 )**: [react-float] feature detect getRootNode ([#25689](react/react#25689)) //<Jan Kassens>// - **[e1dd0a2](react/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([#25692](react/react#25692)) //<Sebastian Markbåge>// - **[c54e354](react/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([#25663](react/react#25663)) //<Mengdi Chen>// Changelog: [General][Changed] - React Native sync for revisions d1e35c7...17f6912 jest_e2e[run_all_tests] Reviewed By: makovkastar Differential Revision: D42804802 fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280
… Bun.serve (#32140) Fixes #32137 ### Repro ```ts Bun.serve({ port: 3000, fetch() { const stream = new ReadableStream({ type: "direct", pull(controller) { controller.write("SHELL"); controller.flush(); setTimeout(() => { // like React resolving a Suspense boundary controller.write("RESOLVED"); controller.end(); }, 50); // returns undefined synchronously }, }); return new Response(stream); }, }); ``` `curl` gets `SHELL` (5 bytes, clean 200) instead of `SHELLRESOLVED`. With react-dom 19's `renderToReadableStream` (its `server.bun.js` build uses exactly this shape), the response ends after the shell flush with the pending marker `<!--$?-->` and the fallback, missing `</body></html>`, and React logs `error: The render was aborted by the server without a reason.` because the server cancels the stream mid-render. Reproduces on 1.0.36 through current main. ### Cause For a direct `ReadableStream`, `readDirectStream` calls `underlyingSource.pull(sink)` once and returns `undefined` when pull returns a non-promise. `Bun.serve`'s `do_render_stream` (src/runtime/server/RequestContext.rs) then has no way to know the stream isn't finished: it hits the `"is in progress, but did not return a Promise. Finalizing request context"` branch, which cancels the stream (aborting React's render) and finalizes the sink, truncating everything the producer would have written through the captured controller. react-dom/server.bun.js depends on the captured-controller pattern: `pull` writes the shell and returns, then resolved Suspense boundaries are written later and `end()` is called when the render completes (react/react#25597, which Bun's SSR guide links). ### Fix Three pieces, each with its own test: 1. **`readDirectStream`** (src/js/builtins/ReadableStreamInternals.ts): when `pull()` returns a non-promise and the stream is still readable, return a promise that settles when the sink closes (`end()`, or abort on client disconnect). Native consumers already handle a returned promise through the same pending path async `pull()` uses, so `Bun.serve` keeps the request context alive and lets the later `end()` complete the response. Client disconnects still tear the stream down through the sink's abort path, which invokes the source's `cancel()` (so React aborts its render on disconnect as before). The async-pull contract (socket terminates when the promise resolves) and the default-stream path are unchanged. 2. **`handle_resolve_stream`** (src/runtime/server/RequestContext.rs): `endFromJS()` can hit transport backpressure right after the HEADERS frame on QUIC and park a `pending_flush` promise while `onWritable` drains the remaining bytes. The resolve path destroyed the sink at that point, discarding the parked bytes and failing the request with `HTTP3ContentLengthMismatch`. This pre-exists the PR (an `async pull()` that calls `end()` synchronously fails the same way over h3 on main) and is the same bug class the undefined-result path already guards (the `effective_result`/`pending_flush` substitution). Now the resolve path waits for the parked flush to settle and re-enters. 3. **`detach()`** (src/codegen/generate-jssink.ts): the controller's detach used `JSC::getCallData` to invoke the stored close callback, which reports an `AsyncContextFrame` (stream constructed inside `AsyncLocalStorage.run()`, a common per-request SSR pattern) as not callable, silently skipping the callback. With the new wait this leaked the request context, sink, and `ReadableStream` on every request. Route through `AsyncContextFrame::call` like the generated `onClose` already does. ### Tests In `test/js/bun/http/serve-direct-readable-stream.test.ts` (all fail on the unfixed build; the first two with `This HTTPResponseSink has already been closed. A "direct" ReadableStream terminates its underlying socket once 'async pull()' returns.`): - sync `pull()` that ends later streams the whole body (also asserts the shell arrives before `end()`, i.e. streaming stays progressive) - sync `pull()` that writes nothing and ends later still responds - `cancel()` fires when the client disconnects while waiting for `end()` - h3: async `pull()` that ends synchronously, and sync `pull()` that ends from a microtask (fail with `HTTP3ContentLengthMismatch` without fix 2) - AsyncLocalStorage variant asserting `heapStats()` ReadableStream counts stay flat (leaks 20/20 without fix 3) Also updated: - `body-stream.test.ts` "Should not crash when not returning a promise when stream is in progress": `toBeOneOf(["hey", ""])` tightened to `toBe("hey")` and the timer replaced with condition-driven `end()`, per review - `serve-response-stream-sink-leak-fixture.ts`: the fixture relied on the old truncating fallback (pull never called `end()`, which now means "keep the response open"); it ends each stream from the client side and still asserts flat memory over 10k requests (delta 0.0 bytes/request) Verified the issue's React repro end to end with [email protected]: the response is the complete 1158-byte document containing `<span>RESOLVED</span>` and closing `</body></html>`, with no abort error. Existing suites (`serve.test.ts`, `bun-server.test.ts`, `body-stream.test.ts` incl. the h3 matrix, `streams.test.js`, `direct-readable-stream.test.tsx`, spawn stdin ReadableStream suites, the serve leak tests) show no new failures against the unfixed build. --------- Co-authored-by: Jarred Sumner <[email protected]>
Add support for Bun server renderer
Summary
This PR adds a streaming React renderer that's optimized for Bun, and a conditional export in
"react-dom"that is respected by thebunruntime.How did you test this change?
The built versions of
build/node_modules/react-dom/esm/react-dom-server.bun.{development|production}.jshave been tested against Bun's React test suite: https://github.com/oven-sh/bun/blob/main/test/bun.js/react-dom.test.tsx. For the moment these files were manually copied over to the Bun repo, if/when this PR is merged we'll switch to an NPM dependency in our test suite.