webstreams: make Blob/File/body streams byte streams so BYOB readers attach - #36643
webstreams: make Blob/File/body streams byte streams so BYOB readers attach#36643robobun wants to merge 4 commits into
Conversation
…attach
Blob.prototype.stream(), Bun.file().stream(), fetch/Request/Response bodies,
and subprocess pipe streams are now readable byte streams, so
getReader({ mode: "byob" }) and new ReadableStreamBYOBReader(stream) work
on them instead of throwing:
TypeError: ReadableStreamBYOBReader needs a ReadableByteStreamController
materializeNativeSource now installs a ReadableByteStreamController for
binary native streams (text-mode native streams from Body.textStream()
keep a default controller since they enqueue JSStrings). When a
pull-into is pending the adapter passes the BYOB request's view through
to the native pull(view, closer) and answers with respond(n); with no
pull-into (default reader) it keeps its own scratch buffer and enqueues
the filled prefix as before.
SourceKind::Native now dispatches from both controller types (binary on
the byte controller, text-mode on the default controller). getReader in
BYOB mode and the ReadableStreamBYOBReader constructor materialize a
NativePending binary stream before locking it. ReadableStream__empty
returns a closed byte stream so an empty Blob's stream also accepts a
BYOB reader.
Fixes #6643
Fixes #12908
Fixes #16402
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughChangesNative stream sources can now use byte controllers and BYOB readers. Pull, close, cancellation, error handling, lazy materialization, and event-loop cleanup support both default and byte controllers. Tests cover Blob, file, fetch, server request, partial-read, and memory behavior. Native BYOB streams
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit 1cf29d4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36643That installs a local version of the PR into your bun-36643 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this swaps every binary native stream (Blob/File/fetch body/subprocess pipes) from a default controller to a byte controller — a broad user-observable behavior change with buffer-detach semantics throughout the native pull adapter. Worth a human look.
Checked: the BYOB respond(n) path guards on m_pendingPullIntos non-empty before responding after an async pull settles; the default-reader enqueue path copies the filled prefix so byte-controller detach doesn't kill scratch-buffer reuse; nativeSourceDropEventLoopRef is now applied on the byte-controller release path too. The removed distinctBuffers.size < 8 assertion in streams-leak.test.ts is replaced by the total-backing-bytes bound, which is the actual regression guard.
Extended reasoning...
Overview
This PR changes materializeNativeSource to install a ReadableByteStreamController (instead of a default controller) for every binary native stream — Blob.prototype.stream(), Bun.file().stream(), fetch/Request/Response bodies, and subprocess pipes — so getReader({mode:'byob'}) and new ReadableStreamBYOBReader(stream) work on them. Text-mode native streams (Body.textStream()) keep the default controller. It touches ~10 C++ files in src/jsc/bindings/webcore/streams/ (the native source adapter, both controllers' dispatch tables, the BYOB reader constructor, generic reader release, and ReadableStream__empty), plus three test files.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The buffer offset/length arithmetic in nativePendingPullIntoView and nativeDecodePullResult uses values already validated by the byte-controller spec ops (m_bytesFilled, m_byteLength, view->length()), and written is clamped to view->length() before use.
Level of scrutiny
High. This is a ~400-line net change to memory-safety-critical JSC binding C++ that changes observable behavior for every native ReadableStream in Bun. Byte controllers detach/transfer chunk buffers on enqueue, which is why the default-reader path had to switch from subarray-of-scratch to copy-into-fresh-Uint8Array — a subtle correctness requirement. The adapter now stores either controller type in one internal field with dynamicDowncast accessors, and every close/error/enqueue/cancel/release path branches on which one is present. The m_pendingIsBYOB flag threads whether an in-flight async pull's pending view is the BYOB pull-into buffer (respond) vs an adapter-owned scratch buffer (enqueue). These are exactly the kinds of state-machine interactions that benefit from a maintainer's eye.
Other factors
- An existing test assertion (
distinctBuffers.size < 8in streams-leak.test.ts) was removed. The rationale is sound — byte-controller enqueue detaches, so each default-reader chunk now owns its own small buffer — and the surviving total-backing-bytes bound (< 4 MB vs the pre-fix ~16 MB) is the actual leak guard. Still, per REVIEW.md "Never silently weaken an existing test", a maintainer should confirm they agree with the trade. - HWM=1 on the byte controller (vs the spec-typical HWM=0 for byte streams) is a deliberate choice explained in a comment; it keeps one proactive pull outstanding so close/error surface after the last read.
- Broad blast radius: every consumer of Blob/file/body streams now sees byte-controller semantics (chunks arrive via detached buffers). The PR's test coverage is good (BYOB fill/loop/constructor/Bun.file/default-reader/release-then-BYOB/empty/multi-byte-element close, plus fetch and Bun.serve request bodies over HTTP/1.1 and HTTP/3), and it reports WPT streams and the node stream suite passing, but CI is still building.
|
CI (build 86954, sha 1cf29d4): all stream/Blob/fetch-body test files this PR touches pass on every lane. The build's only hard failures are unrelated and reported for main-break triage:
Everything else in the annotation is marked flaky and passed on retry. Ready for review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@robobun update PR body wiht some benchmarks (pull first i rebased main) and lets see what the throughput, peak RSS, and cpu usage impact of this change is both when using getReader defaut, and with byob and when using .respondWithValue and not providing a Uint8Array. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp`:
- Around line 690-701: Update nativePendingPullIntoView to calculate the
unfilled tail size before creating the view, and return nullptr when that size
is zero. Keep the existing non-empty view creation and exception handling
unchanged so nativeSourcePullImpl does not treat an exhausted head descriptor as
a BYOB request.
In `@src/jsc/bindings/webcore/streams/JSReadableStream.cpp`:
- Around line 606-609: Introduce a shared materializeForBYOBIfNeeded helper that
owns the NativePending/non-text-mode guard and materialization logic. In
src/jsc/bindings/webcore/streams/JSReadableStream.cpp lines 606-609, replace the
inline guard with the helper; make the same replacement in
src/jsc/bindings/webcore/streams/JSReadableStreamBYOBReader.cpp lines 249-252,
preserving exception propagation.
In `@src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp`:
- Around line 540-553: Reorder the reader-unlinking logic in the stream release
path so both the ControllerKind::Default and ControllerKind::Byte arms clear
stream->m_reader and reader->m_stream before calling the fallible
nativeSourceDropEventLoopRef. Preserve the existing exception propagation while
ensuring a thrown release cannot leave the stream locked.
In `@test/js/web/fetch/blob.test.ts`:
- Around line 596-603: Extract the duplicated BYOB drain loop into a shared
helper in test/harness.ts that tracks the offset, re-views each returned buffer,
and stops at the target length or stream completion. Replace the loops in
test/js/web/fetch/blob.test.ts lines 596-603,
test/js/web/fetch/body-stream.test.ts lines 63-72, and
test/js/web/fetch/body-stream.test.ts lines 88-97 with calls to that helper.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7abc1370-16c6-4e94-a35f-b88c51b24610
📒 Files selected for processing (13)
src/jsc/bindings/webcore/streams/BunStreamSource.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.hsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.cppsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.hsrc/jsc/bindings/webcore/streams/JSReadableStream.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamBYOBReader.cppsrc/jsc/bindings/webcore/streams/ReadableStreamOperations.cppsrc/jsc/bindings/webcore/streams/StreamsForward.hsrc/jsc/bindings/webcore/streams/WebStreamsExports.cppsrc/jsc/bindings/webcore/streams/WebStreamsInternals.htest/js/web/fetch/blob.test.tstest/js/web/fetch/body-stream.test.tstest/js/web/streams/streams-leak.test.ts
…ze helper; unlink reader before event-loop-ref drop; skip transfer on native default-reader enqueue The native byte-controller enqueue fast path skips the spec transfer when fulfilling a default-reader read request (the adapter owns the buffer, no user aliasing), recovering the default-reader throughput regression the spec transfer introduced on the streaming-file path.
There was a problem hiding this comment.
I didn't find bugs, but this switches every native binary stream from a default controller to a byte controller — a wide behavioral surface (buffer-detach semantics, respond()/close ordering, HWM, the new m_pendingIsBYOB async state) that warrants a maintainer's look rather than an auto-approve.
What was reviewed:
- The BYOB pull path (
nativePendingPullIntoView→respond(n)) and the default-reader copy-then-enqueue path — the copy avoids the byte-controller detach of the scratch buffer, andstreams-leak.test.tswas updated to drop thedistinctBuffers.size < 8assertion accordingly. nativeSourceCallClose's byte-controller arm:close()thenrespond(0)on a pending pull-into, and the swallowed throw when the stream is already errored (partial-element close case) — matches spec ordering.- Both
getReader({mode:'byob'})andnew ReadableStreamBYOBReader(stream)materializeNativePendingbefore locking; text-mode andDirectPendingare left to reject.
Extended reasoning...
Overview
This PR changes materializeNativeSource to install a ReadableByteStreamController (instead of a ReadableStreamDefaultController) for every binary native stream — Blob/File/Bun.file/fetch-body/subprocess pipes — so getReader({mode:'byob'}) works. Text-mode native streams keep the default controller. The change spans ~350 lines across 10 C++ files in src/jsc/bindings/webcore/streams/: the adapter now stores an erased controller (defaultController()/byteController() accessors), SourceKind::Native gains byte-controller pull/cancel arms, the pull-result decoder branches on whether a BYOB pull-into is pending (respond(n) vs. copy-and-enqueue), ReadableStream__empty returns a closed byte stream, and readableStreamReaderGenericRelease extracts the updateRef(false) call into nativeSourceDropEventLoopRef for both controller kinds.
Security risks
None identified. No untrusted parsing, no auth/crypto surface. The buffer arithmetic in nativePendingPullIntoView (offset + bytesFilled, byteLength − bytesFilled) reads from a spec-maintained JSPullIntoDescriptor whose invariants are enforced by readableByteStreamControllerPullInto.
Level of scrutiny
High. This is core Web Streams machinery in C++ with GC/buffer-lifetime implications. The default-reader path's semantics change (byte-controller enqueue transfers/detaches the chunk buffer, so the adapter now memcpys the filled prefix into a fresh Uint8Array before enqueue). The close path adds a respond(0) after readableByteStreamControllerClose and conditionally swallows the resulting throw. A new adapter flag m_pendingIsBYOB carries state across an async pull. nativeGetInternalBuffer no longer re-slices the tail — the pending view is now always the full scratch buffer at offset 0. These are all reasonable, but the interactions are subtle enough that a maintainer who owns this subsystem should sign off.
Other factors
- CI is green on all stream/Blob/fetch-body lanes.
- Test coverage is thorough for the new BYOB paths (Blob, Bun.file, fetch response, Bun.serve request, empty Blob, default-then-BYOB, multi-byte-element close).
streams-leak.test.tsdrops thedistinctBuffers.size < 8assertion (each chunk now owns its own small buffer); the total-backing-bytes assertion is preserved. Worth a maintainer confirming this weakening is acceptable.- The comment-cop bot left ~15 inline flags on the new multi-line comments; those appear advisory (the comments are contract documentation, not workaround justifications), but that's a maintainer call.
|
Benchmarks are in the PR body. Summary (release builds, Linux x64, median of 3×15 runs):
Also in 9960e06: trimmed the comment-cop-flagged comments, added the zero-tail guard and |
The direct-fulfil fast path skips readableByteStreamControllerEnqueue's ProcessReadRequestsUsingQueue step; a respond() with readerType=None (released BYOB reader) can leave bytes in m_queue, so gating on m_queue.isEmpty() matches the spec's precondition for the direct-fulfil branch. Adds a BYOB-release-then-default-read byte-order regression guard.
There was a problem hiding this comment.
The queue-ordering fix in 1cf29d4 looks right and the added byte-order regression test covers it. No new issues found this pass, but this is a ~400-line change to the native stream adapter that swaps every binary native source from a default controller to a byte controller — worth a human look, especially the open HWM=1 vs HWM=0 trade-off (BYOB file reads are ~20-30% slower than the default reader) and the weakened distinctBuffers.size < 8 assertion in streams-leak.test.ts.
What was reviewed:
nativeByteControllerEnqueuefast path now gates onm_queue.isEmpty(), matching the spec's precondition for direct-fulfil.nativeDecodePullResultBYOB arm guardsrespond()on!m_pendingPullIntos.isEmpty()so a released reader mid-async-pull doesn't hit the spec assert.readableStreamReaderGenericReleasereordering unlinks the reader before the fallibleupdateRef(false)in both controller arms.nativePendingPullIntoViewreturns nullptr on a zero-tail descriptor so an exhausted head doesn't stall as a BYOB request.
Extended reasoning...
Overview
This PR converts Bun's lazily-materialized native ReadableStream sources (Blob/File/fetch body/Bun.serve request body/subprocess pipes) from ReadableStreamDefaultController to ReadableByteStreamController so getReader({mode:'byob'}) works. ~400 net lines across 11 C++ files in src/jsc/bindings/webcore/streams/ plus 3 test files. The adapter now dispatches to either controller type (default for text-mode, byte for binary), passes the BYOB pull-into's view directly to the native pull(view, closer) and commits with respond(n), and copies the filled prefix into a fresh Uint8Array on the default-reader path (since byte-controller enqueue detaches its argument's buffer).
Since my last review
Commit 1cf29d4 addressed the byte-reordering hazard I flagged by adding && controller->m_queue.isEmpty() to the nativeByteControllerEnqueue fast-path guard, and added a BYOB-release→default-read regression test in blob.test.ts. The author noted the specific interleaving I described isn't empirically reachable against Bun.file().stream() because HWM=1 triggers a proactive scratch-buffer pull first, but the guard is correct regardless and matches the spec's ASSERT(queue empty) precondition. This looks right.
Security risks
None identified. No untrusted-input parsing; the byte-controller spec ops already bounds-check pull-into descriptors. The memcpy in the copy-out path uses std::min(written, view->length()) so an over-reporting native pull can't overrun.
Level of scrutiny
High. This is core streams plumbing reached by every Blob/File/fetch/server body consumer, in memory-safety-sensitive C++ (JSC GC barriers, ArrayBuffer detachment, pull-into descriptor lifecycle). The change is well-tested (WPT streams, blob/body/body-stream suites, leak test) and CI is green, but the surface area — every native stream now goes through a different controller type — and the fast path that bypasses spec TransferArrayBuffer warrant a maintainer's eyes.
Other factors
- Jarred asked for benchmarks; robobun's response leaves the HWM=1 vs HWM=0 choice as an open question (BYOB file reads are slower than default reader with HWM=1 because the proactive pull lands in the queue and forces
fillPullIntoDescriptorFromQueuememcpy). That's a design decision a maintainer should sign off on. streams-leak.test.tsdrops thedistinctBuffers.size < 8assertion in favour of only checking total backing bytes. The rationale (byte-controller path now copies each pull into a fresh small Uint8Array rather than subarraying one 256KB buffer) is sound, but weakening an existing leak-regression assertion is worth an explicit ack.- One unresolved comment-cop nag on the 3-line fast-path comment at BunStreamSource.cpp:394 — cosmetic.
2 participants
What
Blob.prototype.stream(),Bun.file().stream(), fetch/Request/Response bodies, and subprocess pipe streams are now readable byte streams, sogetReader({ mode: "byob" })andnew ReadableStreamBYOBReader(stream)work on them. Previously they threw:The File API requires
Blob.prototype.stream()to return a byte stream, and Node.js / Deno / every browser already do, so zero-copy BYOB read loops that work elsewhere failed only on Bun.How
materializeNativeSourcenow installs aReadableByteStreamControllerfor binary native streams. Text-mode native streams (Body.textStream()) keep aReadableStreamDefaultControllersince they enqueue JSStrings.SourceKind::Nativedispatches from both controller types accordingly.When a BYOB pull-into is pending, the adapter passes the BYOB request's view through to the native
pull(view, closer)and commits withrespond(n); with no pull-into (default reader) it keeps its own scratch buffer andenqueue()s the filled prefix. The default-reader path now copies the filled prefix into a freshUint8Array(n)before enqueue (byte-controller enqueue detaches its chunk's buffer, which would otherwise defeat scratch-buffer reuse).getReader({mode:"byob"})andnew ReadableStreamBYOBReader(stream)materialize aNativePendingbinary stream before locking it.ReadableStream__emptyreturns a closed byte stream so an empty Blob's stream also accepts a BYOB reader and observesdone: true.Tests
test/js/web/fetch/blob.test.tsgains aBlob.prototype.stream() is a byte streamsuite: a BYOB read fills the caller's view, a BYOB loop reassembles the full blob,new ReadableStreamBYOBReader(blob.stream()),Bun.file().stream()BYOB, default reader still works, release-then-BYOB on the same stream, empty Blob, and multi-byte-element BYOB close.test/js/web/fetch/body-stream.test.tsadds fetch-response-body and Bun.serve-request-body BYOB reads over HTTP/1.1 and HTTP/3.All of these throw
TypeErroron main.blob.test.ts(54 tests),body.test.ts(448),body-stream.test.ts(9090),native-source-onclose-leak.test.ts,textstream-wpt.test.ts, the node stream suite, and WPT streams (1175) all pass.Fixes #6643
Fixes #12908
Fixes #16402
Supersedes #33927 (same change, 447 commits behind main with unresolvable conflicts against the adapter's
JSInternalFieldObjectImplrework in #36337 andBody.textStream()in #33825) and #29167 (targeted the since-removedsrc/js/builtins/ReadableStream*.ts).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts test/js/web/streams/streams-leak.test.ts
Benchmarks
Release builds of this PR vs this PR's merge-base on main, Linux x64. Each row is the median of 3 runs × 15 iterations (file) or 20 iterations (Blob).
Blob.stream()(64 MB in-memory, fully-buffered fast path)Default reader is unchanged. BYOB is ~27% faster (one
respond(n)per chunk into the caller's buffer, no per-chunk allocation).Bun.file().stream()(256 MB file, streaming adapter path)Default reader is at parity (the native enqueue fast path skips the spec
TransferArrayBufferwhen fulfilling a default-reader request, since the adapter owns the buffer with no user aliasing; without that fast path the default reader lost ~25% here).BYOB on the streaming-file path is currently slower than the default reader: the byte controller runs with HWM=1 (matching the previous default-controller HWM=1 so a native-side close/error propagates without a pending user read), and that proactive pull lands in the queue between BYOB reads, so each BYOB read is satisfied by
fillPullIntoDescriptorFromQueue(amemcpy) rather than a zero-copyrespond(n). Peak RSS is ~20% lower than the default reader either way.Benchmark script