Repository navigation
fetch: keep textStream() pulling when a native chunk decodes to nothing - #36180
Conversation
Body.textStream() over a native ByteStream (fetch response) stalls forever when two consecutive network reads each decode to the empty string, which happens whenever a multi-byte UTF-8 character is split across three or more delivered chunks (e.g. an emoji trickled byte-at-a-time). The native text-mode adapter's nativeEnqueueTextChunk holds back the incomplete tail and returns without enqueueing, so nothing arms m_pullAgain, and once the one buffered re-pull (from the preceding enqueue) is spent the pull loop stops with a read request still pending and more data still to come. The SourceKind::TextDecode path (textStream() over a user ReadableStream) already handles this by calling callPullIfNeeded on an empty decode; apply the same contract to the native adapter.
|
Status: fix pushed, waiting on CI. Reproduced with: |
|
Warning Review limit reached
Next review available in: 8 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 (2)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-targeted hang fix that mirrors the existing TextDecode sibling.
What was reviewed:
- Confirmed the fix mirrors
textDecodeReadRequestChunkSteps(ReadableStreamOperations.cpp:1085-1088), which already handles the same empty-decode case identically. - Traced
callPullIfNeededbehavior at each of the 6 call sites: inside a pull it setsm_pullAgain; fromnativeSourceStartit's a no-op (m_startedgate); fromonDrainit correctly issues/arms the next pull; theflush=truesites skip it. - Checked the
written == 0numeric-result path innativeDecodePullResult— it doesn't callnativeEnqueueTextChunk, so behaves identically to byte mode there (pre-existing, out of scope).
Extended reasoning...
Overview
Fixes a hang in Body.textStream() over a native fetch body when two consecutive native pulls decode to the empty string (multi-byte UTF-8 char split across ≥3 chunks, or a byte-at-a-time BOM). The 5-line C++ change in nativeEnqueueTextChunk calls readableStreamDefaultControllerCallPullIfNeeded when a non-flush decode yields nothing, re-arming m_pullAgain so the spec pull loop continues. A 7-case test.each in body.test.ts drives a raw chunked net.Server to reproduce the stall.
Security risks
None. No user-controlled input reaches new parsing/allocation; the change only adds a re-pull call on an existing controller. No lifetime, ownership, or exception-scope changes beyond wrapping the tail call in RELEASE_AND_RETURN.
Level of scrutiny
Moderate — C++ JSC streams bindings — but the change is a direct mirror of textDecodeReadRequestChunkSteps at ReadableStreamOperations.cpp:1088, which already handles the identical empty-decode case for the non-native SourceKind::TextDecode path. I verified callPullIfNeeded's behavior at every nativeEnqueueTextChunk call site: during a pull it sets m_pullAgain (JSReadableStreamDefaultController.cpp:534-536); from nativeSourceStart it's gated off by !m_started in shouldCallPull; from onDrain it either issues a pull or arms one; the two flush=true sites (close, buffered fast path) correctly skip it.
Other factors
- Full
body.test.tssuite passed (440 pass, 0 fail); PR notes 5/7 new cases time out on main. - The comment-cop bot's inline note was addressed (comment trimmed in adef51e; thread resolved).
- Tests are hermetic (
net.createServeronport: 0, try/finally cleanup, no external hosts). ThesetImmediateyield between socket writes is used to separate chunks on the wire, not to wait for a condition — coalescing would only weaken (not flake) the test. - Ruled out the
written == 0numeric branch innativeDecodePullResultas a related gap: it never callsnativeEnqueueTextChunkand matches byte-mode behavior, so it's pre-existing and orthogonal.
Follow-up to #36180. Test-only. Adds regression coverage for the additional faces of the native-adapter empty-decode stall, all verified to hang on b22e0e6 (the commit before #36180) and pass on d549845: - **close-after-empty**: a body that ends in (or is only) an incomplete UTF-8 tail reaches the flush decode and closes (`"\ufffd"`) instead of stalling on the final pull. - **BOM-carry then close**: `[EF][BB]` then FIN covers the other null-returning branch of `streamingUTF8Decode` (the possible-BOM hold-back). - **error-after-empty**: a socket drop while the last pull decoded to nothing surfaces as a rejection instead of an idle hang. - **`req.textStream()` server-side**: a chunked upload that splits a code point across chunks completes (same `ByteStream` adapter as the fetch-response path). Also refactors the raw-socket chunked origin into a `rawChunkedServer` helper with `Symbol.asyncDispose` so each case is a three-line `await using`. ``` $ bun bd test test/js/web/fetch/body.test.ts -t textStream 100 pass, 0 fail ``` <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/web/fetch/body.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/web/fetch/body.test.ts bun test v1.4.0 (0cdbb3a) test/js/web/fetch/body.test.ts: (pass) Request > constructor > undefined [5.03ms] (pass) Request > constructor > null [3.19ms] (pass) Request > constructor > string > "" [6.13ms] (pass) Request > constructor > string > "Hello world" [1.53ms] (pass) Request > constructor > string > "🫠" [1.42ms] (pass) Request > constructor > string > "⁉️ " [1.37ms] (pass) Request > constructor > ArrayBuffer > empty buffer [8.69ms] (pass) Request > constructor > ArrayBuffer > small buffer [5.06ms] (pass) Request > constructor > ArrayBuffer > large buffer [4.85ms] (pass) Request > constructor > SharedArrayBuffer > empty buffer [1.88ms] (pass) Request > constructor > SharedArrayBuffer > small buffer [1.76ms] (pass) Request > constructor > SharedArrayBuffer > large buffer [4.13ms] (pass) Request > constructor > Buffer > empty buffer [1.53ms] (pass) Request > constructor > Buffer > small buffer [1.52ms] (pass) Request > constructor > Buffer > large buffer [4.65ms] (pass) Request > constructor > Uint8Array > empty buffer [1.26ms] (pass) Request > constructor > Uint8Array > small buffer [1.78ms] (pass) Request > constructor > Uint8Array > large buffer [3.91ms] (pass) Request > constructor > Uint8ClampedArray > empty buffer [1.63ms] (pass) Request > constructor > Uint8ClampedArray > small buffer [1.55ms] (pass) Request > constructor > Uint8ClampedArray > large buffer [3.41ms] (pass) Request > constructor > Uint16Array > empty buffer [1.67ms] (pass) Request > constructor > Uint16Array > small buffer [1.95ms] (pass) Request > constructor > Uint16Array > large buffer [6.16ms] (pass) Request > constructor > Uint32Array > empty buffer [19.54ms] (pass) Request > constructor > Uint32Array > small buffer [2.43ms] (pass) Request > constructor > Uint32Array > large buffer [10.54ms] (pass) Request > constructor > Int8Array > empty buffer [1.41ms] ( ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/web/fetch/body.test.ts | 96 ++++++++++++++++++++++++++++++++---------- 1 file changed, 73 insertions(+), 23 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/web/fetch/body.test.ts 7 10 0 ``` </details> <!-- robobun:evidence:end -->
…40947) ### Problem - `test/js/web/fetch/body.test.ts` > "rejects a fetch textStream() when the connection drops after an empty decode" fails in the parallel CI batch with `received: ""` instead of `"A"`. The error code is the expected `ECONNRESET`. It passes when it runs alone. - The test server writes `"A"`, `0xF0`, `0x9F` (one `setImmediate` apart) and then destroys the socket. Under load, the HTTP thread sees all of that before the JS thread has read anything. `FetchTasklet::callback` coalesces the updates into one progress update with `fail` set, and `to_body_value` (`src/runtime/webcore/fetch/FetchTasklet.rs:1707`) or `on_body_received` (`:740`) turns the body into an errored stream. The buffered `"A"` is discarded by design. ### Fix - The server now waits until the client has consumed the first chunk before it writes the incomplete UTF-8 tail and drops the socket. A `Promise.withResolvers` resolved from the `for await` body is the handshake. A `finally` resolves it too, so the server still closes the socket if the stream ends early. - The drop still lands after the empty decodes (`0xF0`, `0x9F` decode to nothing), which is what the test guards: the stall fixed in #36180 surfaced as a hang, not a rejection. - Verified: 24 parallel loops of the old test body failed 1400 of 3600 iterations. The new body: 0 of 3600. With `bun test` under the same load: old 32 of 192 failures, new 0 of 192 (release) and 0 of 60 (debug build). The whole file passes with `bun bd test`. ### Background - `res.textStream()` on a fetch response is a native `ByteStream` source in text mode. The HTTP thread delivers body bytes and the terminal result to the JS thread through `FetchTasklet`. - `FetchTasklet::callback` posts at most one task at a time (`has_schedule_callback`). Results that arrive before the JS thread runs that task are merged: body bytes are appended to `scheduled_response_buffer`, and the latest `fail` or `has_more` wins. - When the merged update carries the response head and a failure, the body is `BodyValue::Error` and the bytes are dropped. When it carries a failure after the head was already delivered, `ByteStream::on_data(Err)` rejects the pending pull or, with no pull pending, `append(Err)` clears the buffer (#32662). Whether the consumer sees the bytes before the error depends on timing. <details><summary>Notes</summary> - Repro: `/tmp/repro-textstream.ts` style loop (the test body, 150 iterations) run 24 times in parallel on a 16-core box. Failure rate 24% to 59% per process, all with `received: ""` and `code: "ECONNRESET"`. The same loop with the handshake: 0 failures in 3600 iterations. - The single-process loop (no load) passes 300 of 300 with the old body, which is why the test passes alone. - The other cases in the same `test.each` table end with a clean `0\r\n\r\n`. A coalesced successful end delivers the bytes (`TemporaryAndDone`), so they are not affected. - The data-then-error order is timing dependent in bun for any streaming fetch that fails mid-body. Browsers deliver the bytes received before the error first. This PR does not change that behavior. It only removes the dependence from the test. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/body.test.ts <!-- robobun:evidence:end -->
2 participants
Body.textStream()over a nativeByteStream(a fetch response body) stalls forever when a multi-byte UTF-8 character is split across three or more delivered chunks, i.e. whenever two consecutive native pulls each decode to the empty string.Reproduction
The same origin reads correctly via
res.text(),res.body.pipeThrough(new TextDecoderStream()), andtextStream()over a user-providedReadableStreambody.Cause
nativeEnqueueTextChunkholds back an incomplete UTF-8 tail and returns without enqueueing. In byte mode every pull that resolves with data enqueues something, which tail-callscallPullIfNeededand armsm_pullAgain; in text mode an all-held-back chunk skips that, so once the one buffered re-pull (set by the preceding enqueue'scallPullIfNeeded) is consumed, the spec pull-fulfilled handler seespullAgain == falseand stops. The outstanding read request is never fulfilled andhandle.pull()is never called again.The
SourceKind::TextDecodepath (textDecodeReadRequestChunkSteps, used when the body is already a materialized byteReadableStream) already handles the empty-decode case by callingreadableStreamDefaultControllerCallPullIfNeeded; the native adapter path did not.Fix
When a non-flush decode produces no output, call
readableStreamDefaultControllerCallPullIfNeeded(controller). Inside a pull this setsm_pullAgainso the loop continues; at other call sites (nativeSourceStartwithm_started == false, the push-drivenonDrain) it is either a no-op or correctly issues the next pull.Verification
New
test.eachcases inbody.test.tsdrive a rawnet.Serverthat delivers each body byte as its own HTTP chunk with an event-loop yield between writes, covering: 4-byte char split as[lead][cont][cont cont], 4-byte char byte-at-a-time, 3-byte char byte-at-a-time, and a byte-at-a-time BOM. All four time out on main and pass with the fix.[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