fetch: implement Body.textStream() - #33825
Conversation
|
Updated 10:16 PM PT - Jul 18th, 2026
❌ @robobun, your commit 6b1404c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33825That installs a local version of the PR into your bun-33825 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds ChangestextStream() implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 `@src/jsc/bindings/webcore/streams/StreamsForward.h`:
- Around line 119-120: The comments for SourceKind::TextDecode and the stream
controller setup are stale and describe non-existent types/shapes. Update the
documentation in StreamsForward.h around TextDecode and the controller
algorithmContext comment to match the actual ReadableStreamOperations.cpp
behavior: algorithmContext is the JSReadableStreamDefaultReader and
underlyingObject is the state Uint8Array, with no InternalFieldTuple or
JSStreamTextDecodeContext mentioned. Keep the wording aligned with the existing
WebStreamsInternals.h comment for the same feature.
In `@test/js/web/fetch/body.test.ts`:
- Around line 276-416: The current textStream() coverage in body.test.ts misses
the locked native-request body path that can expose the Body.rs
to_text_readable_stream Locked behavior. Add a test using a
server/request-backed Request or Response body in a pending Locked state, then
call .textStream() and verify subsequent access to .body or a second
.textStream() behaves correctly without corrupting the underlying readable slot.
Use the existing textStream() test group and the fn()/ReadableStream-based
helpers as the reference points when adding the new scenario.
🪄 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: bd5b70fa-7102-4b26-b8cb-042430d8b7c4
📒 Files selected for processing (16)
packages/bun-types/fetch.d.tssrc/jsc/bindings/webcore/streams/BunStreamSource.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.hsrc/jsc/bindings/webcore/streams/JSReadRequest.cppsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.cppsrc/jsc/bindings/webcore/streams/JSReadableStream.hsrc/jsc/bindings/webcore/streams/JSReadableStreamDefaultController.cppsrc/jsc/bindings/webcore/streams/ReadableStreamOperations.cppsrc/jsc/bindings/webcore/streams/StreamsForward.hsrc/jsc/bindings/webcore/streams/WebStreamsExports.cppsrc/jsc/bindings/webcore/streams/WebStreamsInternals.hsrc/jsc/bindings/webcore/streams/WebStreamsMisc.cppsrc/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/response.classes.tstest/js/web/fetch/body.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/Body.rs:1011-1030— TheValue::Lockedarm stores the text-mode stream inlocked.readable(taggedSource::Bytes) and never transitions toValue::Used— so on a fetch response or incoming server request body, afterres.textStream():res.bodyreturns the string stream (consumers expectingUint8Arraychunks get strings),res.bodyUsedis stillfalse, and a secondres.textStream()doesn't throw but instead wraps the string stream in another TextDecode reader that errors on first read with "chunk that is not an ArrayBufferView". The existing "second textStream() call throws" test only coversfn("hello")(theWTFStringImplarm, which correctly transitions toUsed); this arm should likewise transition toValue::Usedand not cache the text stream in the byte-orientedlocked.readableslot.Extended reasoning...
What the bug is
to_text_readable_stream()'sValue::Lockedarm (Body.rs:1011–1030) creates the text-mode native stream viareader.to_text_readable_stream(), then stores its JSValue inlocked.readableas aReadableStream { ptr: Source::Bytes(context_ptr), value: <text stream> }and returns — leaving*selfasValue::Locked. Every other arm of this function (Empty,InternalBlob/WTFStringImpl,Blob) transitions toValue::Used, and the sibling early-return inget_text_stream()(the already-materialized-stream path) also sets*self.get_body_value() = Value::Used. Only this arm — the one that runs for fetch responses and incoming server request bodies — leaves the body in a state where the byte-orientedlocked.readableslot holds aReadableStream<string>.Reachability
A fetch
Responsebody is created asValue::Lockedwithon_start_streamingset andlocked.readableempty (FetchTasklet.rs), and the JSstreamcache slot is unset until.bodyis accessed. So on a fresh fetch response,get_text_stream()at Body.rs:1959–1966 checksget_body_readable_stream(), which returnsNone(JS cache empty at line 1801,locked.readableempty at line 1813), and falls through toto_text_readable_stream()'sValue::Lockedarm. The same is true for an incomingBun.serverequest body before.bodyis touched. This is the primary use case fortextStream(), not an edge case.Consequence 1:
res.bodyreturns a string streamAfter
res.textStream(), the body is stillValue::Lockedwithlocked.readable= the text stream.get_body()(Body.rs:1939–1942) onValue::Lockedcallsget_body_readable_stream(), which now findslocked.readableat line 1813 and returnsreadable.value— the text-mode stream. Sores.bodyyields aReadableStreamwhose chunks are JS strings, notUint8Arrays. Any consumer that expects the spec'dReadableStream<Uint8Array>(e.g.res.body.pipeTo(writableExpectingBytes), orfor await (const chunk of res.body) chunk.byteLength) will misbehave.Consequence 2: second
res.textStream()doesn't throw synchronouslythrow_if_body_unusable()→body_stream_check(is_disturbed || is_locked)findslocked.readable= the text stream. That stream is a freshNativePendingstream with no reader attached (m_readerempty,m_lockedWithoutReader = false,m_disturbed = false,nativeHandleDetached() = false), sois_disturbed || is_lockedisfalse→ no throw. Thenget_body_readable_stream()returns the text stream andtext_decode_from()locks a reader on it. On the first pull,textDecodeReadRequestChunkSteps()receives a JS string chunk,dynamicDowncast<JSArrayBufferView>fails, and the output stream errors with"Body.textStream() received a chunk that is not an ArrayBufferView". The spec requires a synchronousTypeErroron the second call; instead the user gets a stream that errors asynchronously with a confusing message.Consequence 3:
res.bodyUsedisfalseimmediately aftertextStream()get_body_used()→body_stream_check(is_disturbed)on the not-yet-read text stream →false. Per the spec,textStream()disturbs the body's stream, sobodyUsedmust betrueimmediately after. The "marks body as used" test usesfn("hello")(theWTFStringImplarm) and passes; the fetch test only checksbodyUsedafterArray.fromAsynchas consumed the stream, so neither catches this.Step-by-step proof
const res = await fetch(server.url); // body = Value::Locked, locked.readable empty const s1 = res.textStream(); // → get_text_stream: get_body_readable_stream() = None → to_text_readable_stream() // → Value::Locked arm: locked.readable = Strong { Source::Bytes(ctx), <text stream> } // *self stays Value::Locked res.bodyUsed; // false ← should be true res.body; // returns the text stream ← should be a byte stream (or locked/used) const s2 = res.textStream(); // → throw_if_body_unusable: text stream is neither disturbed nor locked → OK // → get_body_readable_stream() = text stream → text_decode_from(text stream) // ← should have thrown TypeError synchronously for await (const _ of s2) {} // → textDecodeReadRequestChunkSteps gets a string chunk → errors: // "Body.textStream() received a chunk that is not an ArrayBufferView"
Why the byte-mode sibling's caching doesn't apply here
to_readable_stream()'sValue::Lockedarm intentionally caches the byte stream inlocked.readableso.bodyis idempotent — accessing.bodytwice returns the sameReadableStream<Uint8Array>. That invariant is exactly what thelocked.readableslot exists for, and is whyget_body()short-circuits through it.textStream()has different semantics: it consumes the body (like.text()), so caching its result in the slot that.body/bodyUsed/throw_if_body_unusableall consult as "the body's byte stream" is a category error.Fix
The
Value::Lockedarm still needs to hand theByteStreamcontext toon_readable_stream_available(fetch/server rely on it to start streaming), but it should not store the text-mode JS stream inlocked.readable, and it should transition the body toValue::Usedbefore returning — matching every other arm and the early-return path inget_text_stream(). One shape: keep theNewSource<ByteStream>setup, callon_readable_stream_availablewith aReadableStream { ptr: Source::Bytes(context_ptr), value: stream_value }constructed on the stack (noStrong::initintolocked.readable), then*self = Value::Usedand returnstream_value. Add a test that calls.textStream()on a fetch response, then assertsres.bodyUsed === trueimmediately and() => res.textStream()throwsTypeError— the current "second textStream() call throws" test only exercises theWTFStringImplpath.
|
Addressed the three findings from the second review plus the CI exception-check failure in 60f32cb:
|
|
On the latest review batch:
|
c17406c to
ba363d3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/webcore/streams/BunStreamSource.cpp (1)
744-758: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing exception check after
nativeEnqueueTextChunkinnativeSourceOnDrain.
streamingUTF8Decodecan throw a catchableOutOfMemoryError(per theexceedsStringLimitguard added for this feature), andnativeEnqueueTextChunkpropagates that viaRETURN_IF_EXCEPTIONrather than absorbing it. Every other call site ofnativeEnqueueTextChunkin this file either checks the exception (with its ownscope) or absorbs it viaDECLARE_TOP_EXCEPTION_SCOPE+Bun__reportError(seenativeSourceCallClose).nativeSourceOnDrainhas no throw scope at all, so a pending exception silently leaks back into the nativeonDraincallback boundary, which isn't expecting one — the same class of bug the PR's own follow-up fix (addingRETURN_IF_EXCEPTIONafterreadableStreamDefaultControllerError) was addressing elsewhere.🐛 Proposed fix mirroring `nativeSourceCallClose`'s absorb pattern
if (adapter->m_textMode) { auto& vm = getVM(globalObject); if (auto* view = dynamicDowncast<JSC::JSArrayBufferView>(chunk)) { std::span<const uint8_t> bytes { static_cast<const uint8_t*>(view->vector()), view->byteLength() }; - nativeEnqueueTextChunk(vm, globalObject, adapter, controller, bytes, /* flush */ false); + auto catchScope = DECLARE_TOP_EXCEPTION_SCOPE(vm); + nativeEnqueueTextChunk(vm, globalObject, adapter, controller, bytes, /* flush */ false); + if (catchScope.exception()) [[unlikely]] { + JSValue thrown = takeAbruptCompletion(globalObject, catchScope); + if (!thrown.isEmpty()) + Bun__reportError(globalObject, JSValue::encode(thrown)); + } } return; }As per path instructions: "In C++ code that can enter JS, check for exceptions after every call that can throw or run user code before using the result."
🤖 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 `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp` around lines 744 - 758, The nativeSourceOnDrain path in BunStreamSource.cpp calls nativeEnqueueTextChunk without any exception handling, so a thrown OutOfMemoryError can escape the onDrain boundary. Update nativeSourceOnDrain to use a throw scope or otherwise check and handle the pending exception immediately after nativeEnqueueTextChunk, matching the exception handling pattern used by nativeSourceCallClose and the other nativeEnqueueTextChunk call sites in this file.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/webcore/streams/BunStreamSource.cpp`:
- Around line 744-758: The nativeSourceOnDrain path in BunStreamSource.cpp calls
nativeEnqueueTextChunk without any exception handling, so a thrown
OutOfMemoryError can escape the onDrain boundary. Update nativeSourceOnDrain to
use a throw scope or otherwise check and handle the pending exception
immediately after nativeEnqueueTextChunk, matching the exception handling
pattern used by nativeSourceCallClose and the other nativeEnqueueTextChunk call
sites in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c59ba589-2323-4232-a8e9-e5c0e97f204e
📒 Files selected for processing (21)
packages/bun-types/fetch.d.tssrc/jsc/bindings/webcore/streams/BunStreamConsumers.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.hsrc/jsc/bindings/webcore/streams/JSReadRequest.cppsrc/jsc/bindings/webcore/streams/JSReadableByteStreamController.cppsrc/jsc/bindings/webcore/streams/JSReadableStream.hsrc/jsc/bindings/webcore/streams/JSReadableStreamDefaultController.cppsrc/jsc/bindings/webcore/streams/ReadableStreamOperations.cppsrc/jsc/bindings/webcore/streams/StreamQueue.hsrc/jsc/bindings/webcore/streams/StreamsForward.hsrc/jsc/bindings/webcore/streams/WebStreamsExports.cppsrc/jsc/bindings/webcore/streams/WebStreamsInternals.hsrc/jsc/bindings/webcore/streams/WebStreamsMisc.cppsrc/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/response.classes.tstest/js/web/fetch/body.test.tstest/js/web/fetch/wpt/textstream-wpt.test.tstest/js/web/fetch/wpt/textstream.any.js
|
On the |
|
CI on d85e3b2 is red on Windows lanes only, all unrelated to this diff:
None of these touch fetch/body/streams. The textStream tests (76 in body.test.ts, 14 WPT) pass on all lanes that ran. Already used the one re-roll earlier; ready for a maintainer to rerun or merge. |
|
At 6b1404c (rebased onto f5ba155; all review nits addressed). 88 textStream + 14 WPT + 436 body.test.ts + 1175 wpt-streams pass locally; clippy/ts-types green. All review threads resolved. Build 75623: only hard-fail is |
There was a problem hiding this comment.
No new issues found on the latest revision, but this is a ~1.3k-line native feature touching the streams GC/re-entrancy machinery, Body/RequestContext lifecycle, and a hand-rolled streaming UTF-8 decoder — worth a human pass on the overall design and the deferred ReadableStreamTag__tagged edge case.
Extended reasoning...
Overview
Implements Body.textStream() on Request/Response per the fetch spec. Rather than body.pipeThrough(new TextDecoderStream()), each body backing decodes directly: in-memory bodies enqueue a single decoded string; native byte sources (fetch/server) run in a text-mode JSNativeStreamSourceAdapter; already-materialized byte streams use a new SourceKind::TextDecode whose pull issues ReadRequestKind::TextDecode reads on a locked source reader. Adds a new streamingUTF8Decode helper (BOM stripping, WHATWG-aligned trailing-byte hold-back, U+FFFD replacement), a 4-byte inline decode state on SourceAlgorithmSlots/JSNativeStreamSourceAdapter, three new FFI exports, and ~570 lines of tests including a vendored WPT.
Files touched: 8 C++ streams files (BunStreamSource, ReadableStreamOperations, JSReadRequest, JSReadableStream, controllers, WebStreamsMisc/Exports/Internals, StreamQueue, StreamsForward), 3 Rust files (Body.rs, ReadableStream.rs, RequestContext.rs), response.classes.ts, fetch.d.ts, and tests.
Security risks
No auth/crypto/permissions surface. The main risk class is memory safety in the C++ streams layer — the review history surfaced (and fixed) three separate user-reachable segfault paths from concurrent-read / re-entrant-close / cancel-after-release interleavings, plus a Bun.serve request-body abort hang. All are now guarded (reader && reader->m_stream at every release site, canCloseOrEnqueue early returns, end_request_streaming fallback to request_body_readable_stream_ref) and covered by regression tests. streamingUTF8Decode has an exceedsStringLimit guard and the empty-span memcmp UB is fixed.
Level of scrutiny
High. This is new user-facing API implemented across the JSC-bindings/Rust FFI boundary in the streams subsystem — the most re-entrancy- and GC-sensitive part of the runtime. It adds a new SourceKind/ReadRequestKind variant threaded through every controller switch, mutates the Value::Locked → Value::Used body lifecycle, and adds a fallback abort path in RequestContext::end_request_streaming. The direct-decode design (vs. spec's pipeThrough(TextDecoderStream)) is a deliberate architectural choice a maintainer should sign off on.
Other factors
- The PR went through 6 review iterations; every 🔴/🟡 inline finding is resolved with a corresponding regression test.
- One edge case was explicitly deferred by the author (
ReadableStreamTag__taggednot gated onm_nativeTextMode, sonew Response(res.textStream())/fetch({body: textStream})on a native-backed text stream re-tags as raw Bytes) because guarding it brokeFetchTasklet's ByteStream re-derivation — noted as a follow-up. - The string-body BOM-stripping deviation was intentionally left to match existing
.text()behavior. - Test coverage is thorough (concurrent reads, re-entrant close, cancel-after-close, mid-upload abort, invalid-prefix hold-back, detached buffers, WPT), and the gate confirms fail-without-fix / pass-with-fix on both ASAN debug and release.
|
@robobun rebase and fix conflicts |
5f1bc1d to
a90ccfb
Compare
|
Rebased onto main (5d673d9). The only textual conflict was in
|
Convert an abrupt completion from the TextDecode read (e.g. the exceedsStringLimit OutOfMemoryError inside streamingUTF8Decode) into a rejected promise so onRSDefaultControllerPullRejected runs and errors the output stream, instead of leaving m_pulling stuck and the pending read unsettled. Matches the nativeSourcePull / SourceKind::JavaScript catch-and-convert pattern.
…ules incompleteTrailingUTF8() now rejects never-valid leads (0xC0-0xC1, 0xF5-0xF7) and applies the per-lead second-byte range (0xE0: >=0xA0; 0xED: <=0x9F; 0xF0: >=0x90; 0xF4: <=0x8F), so a surrogate / overlong / out-of-range prefix at a chunk boundary is replaced immediately instead of held back. Also await the .rejects assertion in the locks-previous-body test.
…reader on termination - nativePtrForJS() returns empty when m_nativeTextMode is set so Readable.fromWeb(res.textStream()) falls back to a reader-based path and sees decoded strings instead of raw bytes. ReadableStreamTag__tagged reads the raw slot so the fetch/server push side is unaffected. - Release the source reader via readableStreamDefaultReaderRelease() on every TextDecode terminal path (closeSteps, cancelAlgorithm, errorSteps, non-BufferSource error branch) so source.locked becomes false after the text stream terminates, matching pipeTo's Finalize step.
…sts by body type
- Add !joined.empty() before the BOM-prefix memcmp so a {nullptr, 0}
span (detached first chunk) never passes nullptr to memcmp.
- The four server/fetch textStream() tests that do not reference fn now
only register under the relevant describe (Response for fetch-response
tests, Request for server-side-request tests) instead of running twice.
…ader closeSteps can release the source reader while the output controller still has a queued flush chunk (ClearAlgorithms only runs when the queue is empty). A subsequent output cancel() then dereferences the released reader's null m_stream. Guard on reader && reader->m_stream and return a fulfilled promise when the source is already terminal.
The closeSteps flush enqueue can tail-call callPullIfNeeded and re-enter closeSteps (source already Closed), whose inner call releases the source reader; the outer call's release then dereferences the null m_stream. Guard every readableStreamDefaultReaderRelease(source_reader) site on reader && reader->m_stream so a second release is a no-op. Added a regression test for the re-entrant close path.
…queue helper nativeEnqueueTextChunk now takes the StreamingUTF8DecodeState by reference instead of the adapter, so the fully-buffered fast path in materializeNativeSource can share it. All call sites pass view->span() (or .span().first(count) for the partial-write case) instead of hand-building the span from vector()+byteLength().
Keeps the existing byte-mode partial-fill path (the two uint8Subarray calls) at its original indentation so it no longer appears in this diff. The text-mode path allocates no subarray objects: it decodes directly from view->span().first(count) and reuses the whole view for the next pull.
nativePtrForJS() already returns empty for text-mode streams, so the explicit check immediately before it is dead.
457c8a5 to
4045358
Compare
2 participants
Implements the
textStream()method on the Body mixin (Request and Response), returning aReadableStream<string>of the body decoded as UTF-8 text.Approach
Rather than materializing a byte
ReadableStreamand piping it through aTextDecoderStream, each body backing decodes directly:ReadableStream__fromDecodedText).ByteStream, file loader) run in a text-modeJSNativeStreamSourceAdapterthat UTF-8-decodes each pulled span before enqueueing. The streaming-decode state (3 held-back bytes + BOM-seen) lives inline on the adapter; no extra GC cells.ReadableStream, or.bodywas accessed first) use a newSourceKind::TextDecodewhose pull issues aReadRequestKind::TextDecoderead on the source reader; chunk steps decode and enqueue strings. The decode state is a 4-byteUint8Arraystored in the controller's existingunderlyingObjectslot.streamingUTF8Decode(WebStreamsMisc.cpp) joins held-back bytes from a previous chunk, strips a single leading BOM per stream, holds back a trailing incomplete sequence, and decodes viaWTF::String::fromUTF8ReplacingInvalidSequences. Held-back bytes are always[lead(>=0xC0), cont*(0x80..0xBF)]so the length is derived from the first zero byte.Verification
Covers: string/buffer/null/undefined bodies, user ReadableStream bodies, multi-byte characters split across chunks, leading BOM stripping, invalid-sequence replacement, fetch response bodies (native ByteStream path), body-unusable/locked error handling, error propagation, cancel propagation, Content-Type charset is ignored.
[review] gate passed · iteration 14 · 20 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 17 passed · 0 rejected · iteration 14
evidence per changed file