Repository navigation
ReadableStream .text(), .json(), .blob(), .bytes() - #20694
Merged
Merged
Conversation
Collaborator
|
Updated 12:36 AM PT - Jun 28th, 2025
✅ @Jarred-Sumner, your commit bcf2da7f45f1250d77b6eb182205152ac9e662dd passed in 🧪 To try this PR locally: bunx bun-pr 20694That installs a local version of the PR into your bun-20694 --bun |
Contributor
|
probably way out of scope, but a |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 4, 2026
…41387) ### Problem - `test/js/bun/spawn/readablestream-helpers.test.ts` fails at random. On PR builds 110091 (debian 13 aarch64) and 110186 (windows 11 aarch64), a case got `Failed to parse JSON` where it expected `JSON Parse error: Expected '}'`, or the reverse. - `json()` on a stream that holds the whole body parses it in `BunString__toJSON` (`src/jsc/bindings/BunString.cpp:522`) or `EncodedSlice__toJSONObject` (`src/jsc/bindings/bindings.cpp:2748`). These call `JSC::JSONParse` and throw the generic message. A streamed body gets the `JSON.parse` message. - For `proc.stdout`, timing selects the path. #20694 pinned one message per case. Since #41204, one failure is final. ### Fix - Both helpers call `JSONParseWithException`, and parse `""` for an empty input. For the same text, they throw the SyntaxError that `JSON.parse` throws. The removed "See #5859" comment is stale (see Notes). - `Response.json()`, `Request.json()`, `Blob.json()` and `Bun.file().json()` change the same way. The fetch spec rethrows the `JSON.parse` exception. Node does too. - `scripts/verify-baseline-static/allowlist-x64.txt` gets `llint_op_jmp_wide32`, a documented LLInt decode false positive that moved with the code layout (see Notes). - Verified on the ASAN build with `BUN_JSC_validateExceptionChecks=1`: `readablestream-helpers.test.ts` (new cases), `body.test.ts`, `serve.test.ts`. Self-reviewed: 5 concerns raised, 4 addressed. ### Background - A stream that Bun creates has a native source. A Blob source returns the whole body at once, so `json()` tries it first. - `JSC::JSONParse` returns an empty value on a syntax error and throws nothing. - Bun creates `proc.stdout` when JS first reads it. If the pipe is at EOF then, it is a Blob stream. `Bun.spawn()` reads the pipe once before it returns, and on Windows `exited` can resolve before EOF. <details><summary>Notes</summary> - Reproduction on the released build (1.4.1): - `new Blob([s]).stream().json()` rejects with `Failed to parse JSON`. A stream with a JavaScript source rejects with `JSON Parse error: Expected '}'`. - `proc.stdout.json()` with no await after `Bun.spawn()`: 11 of 50 runs took the buffered path when the parent and a fast child (`/bin/echo`) share one CPU. This is the debian case. - After `await proc.exited`, a grandchild that keeps the pipe open makes the streamed path run. This is the Windows case. - Empty input: `JSONParseWithException` throws nothing for a null string, and an empty input converts to one. A child that printed only a byte order mark got `JSON Parse error: Unexpected EOF` on the streamed path and `Failed to parse JSON` on the buffered path. Both paths now give the first. A zero-byte stdout was already the same on both paths. - #5859: in 2023 the helper caught the exception and returned it as a value. Today both helpers use a throw scope. The #5859 tests pass with `BUN_JSC_validateExceptionChecks=1`. - Baseline scan: the `:linux: x64-musl - verify-baseline` step flagged one `RDPMC` in `llint_op_jmp_wide32`. Each LLInt opcode handler starts with an embedded 4-byte opcode id, and the linear-sweep decoder desyncs on it. The allowlist already covers two sibling handlers for this reason. The same scan lists those two entries as stale, so the desync moved to this handler. The emulator phase of the same job passes, and the step passed on main at the same base commit. The three LLInt entries are blanket passes, as the triage guide in `scripts/verify-baseline-static/CLAUDE.md` says for a data-in-.text misdecode. - The helpers keep `Failed to parse JSON` for a string that failed to convert (a dead string, or a failed allocation). - The empty-body checks in `Blob.rs` run before the helpers. They resolve `null` or throw `Unexpected end of JSON input`. That is #24955, and this PR does not change them. - #33658 (open since July) makes the same change to the two helpers, and also removes the empty-body checks for #24955. This PR supersedes the helper half of #33658. After it lands, #33658 only needs its `Blob.rs` change. - Self-review, 5 concerns. Addressed: the narrowed claim, the empty-input case, the note that the failures were on PR builds, and the relation to #33658. Not addressed: `Blob::to_json_with_bytes` decodes a body that starts with FF FE as UTF-16LE, and the streamed path decodes it as UTF-8. That difference is separate from this change. - Tests on the debug ASAN build. The first three also ran with `BUN_JSC_validateExceptionChecks=1`. - `test/js/bun/spawn/readablestream-helpers.test.ts`: 32 pass. The released build fails 3 (the two new cases and `(after exited)`). - `test/js/web/fetch/body.test.ts`: 476 pass. The released build fails 34 (17 inputs, for `Request` and `Response`). - `test/js/bun/http/serve.test.ts`: 295 pass, 2 fail. The 2 failures (`root range port(#7187)` and `/bun:info to loopback clients`) also fail on the released build in this environment, which runs as root. - Also pass: `bun-file.test.ts`, `regression/issue/02367.test.ts`, `express.json.test.ts`, `spawn.ipc.test.ts`, `spawn.ipc.bun-node.test.ts`, and the json cases of `fetch.test.ts`. The one failure there, `bad permissions throws`, also fails as root on the released build. </details> <!-- robobun:evidence:begin --> --- **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/body.test.ts, test/js/bun/http/serve.test.ts <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds .text(), .json(), .blob(), .bytes() to Bun.spawn() stdout
Questions: