Fixes #6200 - #6983
Merged
Merged
Fixes #6200#6983
Conversation
Contributor
|
❌ @Jarred-Sumner 1 files with test failures on bun-darwin-aarch64:
|
Contributor
|
❌ @Jarred-Sumner 7 files with test failures on bun-darwin-x64-baseline:
|
This was referenced Nov 28, 2023
This was referenced Apr 30, 2024
This was referenced May 14, 2024
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…esponse (#42573) ### Problem - With the built-in `node-fetch` and `undici` shims, `res.body` emits `'error'` after a body method: `Invalid state: ReadableStream is locked` from `resume()`, `pipe()`, `for await`, and `Cannot cancel a locked ReadableStream` from `destroy()` (for example `finally { body.destroy() }`). Bun 1.4.2 and Node emit `'end'`, `'close'`. Regression from #42116 (not released). No user reported it. - Cause: `ReadableFromWeb` (`src/js/internal/webstreams_adapters.ts`) calls `stream.getReader()` in the first `_read()` and `stream.cancel()` in `_destroy()`. Since #42116 a body method keeps the web stream locked, so both throw. ### Fix - `ReadableFromWeb` takes a `responseBody` option. Both shims set it, `Readable.fromWeb()` does not. If the wrapper has not opened the web stream and the stream is locked, the Response took the body: `_read()` pushes `null`, `_destroy()` skips `cancel()`. - The check reads `stream.locked`, not `bodyUsed`: after an aborted `text()`, `bodyUsed` is `false` and the stream is locked. - `node-fetch`: the body methods no longer read `this.body` first, so the five overrides are deleted. `clone()` drops the cached node stream, because the body moves to a new web stream. - Verified: `test/js/node/http/node-fetch.test.js`, `test/js/first_party/undici/undici.test.ts` (27 new tests, 25 fail with main's `src/`). Self-reviewed: 3 concerns raised, 3 addressed. ### Background - Both shims hand out the body as a node `Readable` (`ReadableFromWeb`) around the web `ReadableStream` of a Bun `Response`. `text()`, `json()`, ... are the native methods and read the web stream directly. - `ReadableFromWeb` opens the web stream lazily, so a body method can take it while the wrapper has no reader. - Real `node-fetch` and `undici` read that node stream in `text()`, so it is ended afterwards. <details><summary>Notes</summary> **Origin.** Found during the review of #42516. That PR lists this symptom as tracked separately. **Reproduction.** ```js import nodeFetch from "node-fetch"; await using server = Bun.serve({ port: 0, fetch: () => new Response("hello") }); const res = await nodeFetch(server.url); const body = res.body; await res.arrayBuffer(); body.on("error", e => console.log("error:", e.message)).on("end", () => console.log("end")); body.resume(); // 1.4.2: end // main: error: Invalid state: ReadableStream is locked ``` **Behaviour per case**, both shims, checked against a 1.4.2 binary. "clean" means `end`, `close` for `resume()`, `close` for `destroy()`, and no chunks for `for await`. | case | 1.4.2 | main | this PR | | --- | --- | --- | --- | | `body` taken, then `text()` / `json()` / `arrayBuffer()` / `blob()` / `formData()` / `buffer()` / `bytes()`, then `resume()`, `destroy()` or `for await` | clean | `error` | clean | | body method first, then `res.body` for the first time | clean (`bytes()` throws) | `error` | clean | | `text()` aborted by the signal (or `AbortSignal.timeout()`), then `resume()` / `destroy()` | clean | `error` | clean | | `formData()` rejects the content type, then `resume()` / `destroy()` | clean | `error` | clean | | `json()` rejects with a `SyntaxError`, then `resume()` / `destroy()` | clean | `error` | clean | | `text()` still pending, then `resume()` / `destroy()` | `error` (locked) | `error` | clean, and `text()` resolves with the full body | | `body` taken, then `res.clone()`, then read `res.body` | throws (locked by `tee()`) | throws | `res.body` is a new node stream and delivers the body. The old node stream ends. | | `res.body` streamed with `for await`, `on("data")`, `pipe()` | works | works | works | | `destroy()` on a body that nothing read | cancels the fetch | same | same | **Why `stream.locked`.** The first version of this change asked the Response for `bodyUsed`. That misses every body method that rejects after it took the stream: on main `bodyUsed` stays `false` there while the stream stays locked (node reports `true` and locked). The lock of the stream itself is what makes `getReader()` and `cancel()` throw, so the wrapper checks that. **Why the overrides are deleted.** #6983 (for #6200) made `text()`, `json()`, `arrayBuffer()`, `blob()`, `formData()` read `this.body` first, because a wrapper made after the consume crashed in the stream code of that time. Such a wrapper now ends without an error, and the test for each body method reads `res.body` for the first time after the method. Without the preload a plain `await res.json()` does not create a web stream and a node stream that nothing reads. **`clone()`.** `super.clone()` moves the body of the original to a new web stream (a `tee()` branch, or a fresh stream over the same bytes). The cached wrapper held the old stream, so `res.body` taken before `clone()` threw `ReadableStream is locked` on read after it, also on 1.4.2. With the lock check it would have ended without data instead. `clone()` now clears the cache. Real `node-fetch` also replaces `body` with a new `PassThrough` in `clone()`. **A body method that fails** (abort, network error, bad JSON) leaves the node stream ended without an error. The rejected promise of the body method carries the error. **Not in this PR.** On main `bodyUsed` is `false` after a body method that rejects when `.body` was read before it. That is native (`src/runtime/webcore/Body.rs`) and #35847 covers the `formData()` part. **Suites run** on the debug build: `node-fetch.test.js`, `undici.test.ts`, `node-fetch-cjs.test.js`, `node-fetch-primordials.test.ts`, `undici-primordials.test.ts`, `node-stream.test.js`, `node-http-agent-free-socket.test.ts`, regression `26225`, `014865`, `04947`, and the Node `test-stream-readable-from-web-termination.js`, `test-readable-from-web-enqueue-then-close.js`, `test-whatwg-webstreams-adapters-to-*.js` files. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 2 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 25 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [68.88ms] (pass) undici > request > should error when body has already been consumed [13.68ms] 97 | const url = new URL(method, server.url).href; 98 | 99 | for (const [action, expected] of bodyActions) { 100 | const { body } = await request(url); 101 | await body[method](); 102 | expect(await eventsUntilClose(body, () => body[action]())).toEqual([...expected]); ^ error: expect(received).toEqual(expected) [ - "end", + [TypeError: Invalid state: ReadableStream is locked], "close", ] - Expected - 1 + Received + 1 at <anonymous> (/workspace/bun/test/js/first_party/undici/undici.test.ts:102:72) (fail) undici > request > body stream when a body method took the r ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (7b5b9c1) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [2.61ms] (pass) undici > request > should error when body has already been consumed [0.37ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [5.40ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [1.87ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [2.11ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [2.05ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [1.75ms] (pass) undici > request > body stream when a body method took the response > resume() leaves a body method that is still reading alone [2.04ms] (pass) undici > request > body stream when a body method took the response > destroy() leaves a body method that ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [78.45ms] (pass) undici > request > should error when body has already been consumed [15.95ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [258.02ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [57.58ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [50.54ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [48.81ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [48.04ms] (pass) undici > request > body stream when a bo ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 639ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/21] gen JS modules (bundle-modules) Preprocess modules (7038ms) Bundle modules (48ms) Postprocesss modules (24ms) Bundle Functions (489ms) Generate Code (42ms) [7.65s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/6] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compili ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/webstreams_adapters.ts | 30 ++++-- src/js/thirdparty/node-fetch.ts | 36 +------ src/js/thirdparty/undici.js | 2 +- test/js/first_party/undici/undici.test.ts | 127 +++++++++++++++++++++++++ test/js/node/http/node-fetch.test.js | 150 ++++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 41 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/webstreams_adapters.ts 5 11 17 src/js/thirdparty/node-fetch.ts 3 3 17 src/js/thirdparty/undici.js 2 3 17 test/js/first_party/undici/undici.test.ts 1 1 12 test/js/node/http/node-fetch.test.js 2 3 17 ``` </details> <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…esponse (oven-sh#42573) ### Problem - With the built-in `node-fetch` and `undici` shims, `res.body` emits `'error'` after a body method: `Invalid state: ReadableStream is locked` from `resume()`, `pipe()`, `for await`, and `Cannot cancel a locked ReadableStream` from `destroy()` (for example `finally { body.destroy() }`). Bun 1.4.2 and Node emit `'end'`, `'close'`. Regression from oven-sh#42116 (not released). No user reported it. - Cause: `ReadableFromWeb` (`src/js/internal/webstreams_adapters.ts`) calls `stream.getReader()` in the first `_read()` and `stream.cancel()` in `_destroy()`. Since oven-sh#42116 a body method keeps the web stream locked, so both throw. ### Fix - `ReadableFromWeb` takes a `responseBody` option. Both shims set it, `Readable.fromWeb()` does not. If the wrapper has not opened the web stream and the stream is locked, the Response took the body: `_read()` pushes `null`, `_destroy()` skips `cancel()`. - The check reads `stream.locked`, not `bodyUsed`: after an aborted `text()`, `bodyUsed` is `false` and the stream is locked. - `node-fetch`: the body methods no longer read `this.body` first, so the five overrides are deleted. `clone()` drops the cached node stream, because the body moves to a new web stream. - Verified: `test/js/node/http/node-fetch.test.js`, `test/js/first_party/undici/undici.test.ts` (27 new tests, 25 fail with main's `src/`). Self-reviewed: 3 concerns raised, 3 addressed. ### Background - Both shims hand out the body as a node `Readable` (`ReadableFromWeb`) around the web `ReadableStream` of a Bun `Response`. `text()`, `json()`, ... are the native methods and read the web stream directly. - `ReadableFromWeb` opens the web stream lazily, so a body method can take it while the wrapper has no reader. - Real `node-fetch` and `undici` read that node stream in `text()`, so it is ended afterwards. <details><summary>Notes</summary> **Origin.** Found during the review of oven-sh#42516. That PR lists this symptom as tracked separately. **Reproduction.** ```js import nodeFetch from "node-fetch"; await using server = Bun.serve({ port: 0, fetch: () => new Response("hello") }); const res = await nodeFetch(server.url); const body = res.body; await res.arrayBuffer(); body.on("error", e => console.log("error:", e.message)).on("end", () => console.log("end")); body.resume(); // 1.4.2: end // main: error: Invalid state: ReadableStream is locked ``` **Behaviour per case**, both shims, checked against a 1.4.2 binary. "clean" means `end`, `close` for `resume()`, `close` for `destroy()`, and no chunks for `for await`. | case | 1.4.2 | main | this PR | | --- | --- | --- | --- | | `body` taken, then `text()` / `json()` / `arrayBuffer()` / `blob()` / `formData()` / `buffer()` / `bytes()`, then `resume()`, `destroy()` or `for await` | clean | `error` | clean | | body method first, then `res.body` for the first time | clean (`bytes()` throws) | `error` | clean | | `text()` aborted by the signal (or `AbortSignal.timeout()`), then `resume()` / `destroy()` | clean | `error` | clean | | `formData()` rejects the content type, then `resume()` / `destroy()` | clean | `error` | clean | | `json()` rejects with a `SyntaxError`, then `resume()` / `destroy()` | clean | `error` | clean | | `text()` still pending, then `resume()` / `destroy()` | `error` (locked) | `error` | clean, and `text()` resolves with the full body | | `body` taken, then `res.clone()`, then read `res.body` | throws (locked by `tee()`) | throws | `res.body` is a new node stream and delivers the body. The old node stream ends. | | `res.body` streamed with `for await`, `on("data")`, `pipe()` | works | works | works | | `destroy()` on a body that nothing read | cancels the fetch | same | same | **Why `stream.locked`.** The first version of this change asked the Response for `bodyUsed`. That misses every body method that rejects after it took the stream: on main `bodyUsed` stays `false` there while the stream stays locked (node reports `true` and locked). The lock of the stream itself is what makes `getReader()` and `cancel()` throw, so the wrapper checks that. **Why the overrides are deleted.** oven-sh#6983 (for oven-sh#6200) made `text()`, `json()`, `arrayBuffer()`, `blob()`, `formData()` read `this.body` first, because a wrapper made after the consume crashed in the stream code of that time. Such a wrapper now ends without an error, and the test for each body method reads `res.body` for the first time after the method. Without the preload a plain `await res.json()` does not create a web stream and a node stream that nothing reads. **`clone()`.** `super.clone()` moves the body of the original to a new web stream (a `tee()` branch, or a fresh stream over the same bytes). The cached wrapper held the old stream, so `res.body` taken before `clone()` threw `ReadableStream is locked` on read after it, also on 1.4.2. With the lock check it would have ended without data instead. `clone()` now clears the cache. Real `node-fetch` also replaces `body` with a new `PassThrough` in `clone()`. **A body method that fails** (abort, network error, bad JSON) leaves the node stream ended without an error. The rejected promise of the body method carries the error. **Not in this PR.** On main `bodyUsed` is `false` after a body method that rejects when `.body` was read before it. That is native (`src/runtime/webcore/Body.rs`) and oven-sh#35847 covers the `formData()` part. **Suites run** on the debug build: `node-fetch.test.js`, `undici.test.ts`, `node-fetch-cjs.test.js`, `node-fetch-primordials.test.ts`, `undici-primordials.test.ts`, `node-stream.test.js`, `node-http-agent-free-socket.test.ts`, regression `26225`, `014865`, `04947`, and the Node `test-stream-readable-from-web-termination.js`, `test-readable-from-web-enqueue-then-close.js`, `test-whatwg-webstreams-adapters-to-*.js` files. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 2 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 25 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [68.88ms] (pass) undici > request > should error when body has already been consumed [13.68ms] 97 | const url = new URL(method, server.url).href; 98 | 99 | for (const [action, expected] of bodyActions) { 100 | const { body } = await request(url); 101 | await body[method](); 102 | expect(await eventsUntilClose(body, () => body[action]())).toEqual([...expected]); ^ error: expect(received).toEqual(expected) [ - "end", + [TypeError: Invalid state: ReadableStream is locked], "close", ] - Expected - 1 + Received + 1 at <anonymous> (/workspace/bun/test/js/first_party/undici/undici.test.ts:102:72) (fail) undici > request > body stream when a body method took the r ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (7b5b9c1) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [2.61ms] (pass) undici > request > should error when body has already been consumed [0.37ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [5.40ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [1.87ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [2.11ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [2.05ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [1.75ms] (pass) undici > request > body stream when a body method took the response > resume() leaves a body method that is still reading alone [2.04ms] (pass) undici > request > body stream when a body method took the response > destroy() leaves a body method that ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [78.45ms] (pass) undici > request > should error when body has already been consumed [15.95ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [258.02ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [57.58ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [50.54ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [48.81ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [48.04ms] (pass) undici > request > body stream when a bo ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 639ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/21] gen JS modules (bundle-modules) Preprocess modules (7038ms) Bundle modules (48ms) Postprocesss modules (24ms) Bundle Functions (489ms) Generate Code (42ms) [7.65s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/6] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compili ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/webstreams_adapters.ts | 30 ++++-- src/js/thirdparty/node-fetch.ts | 36 +------ src/js/thirdparty/undici.js | 2 +- test/js/first_party/undici/undici.test.ts | 127 +++++++++++++++++++++++++ test/js/node/http/node-fetch.test.js | 150 ++++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 41 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/webstreams_adapters.ts 5 11 17 src/js/thirdparty/node-fetch.ts 3 3 17 src/js/thirdparty/undici.js 2 3 17 test/js/first_party/undici/undici.test.ts 1 1 12 test/js/node/http/node-fetch.test.js 2 3 17 ``` </details> <!-- 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
1 participant
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.
What does this PR do?
Fixes #6200
The
bodygetter in ournode-fetchpolyfill now works as expected. Previously, an exception was thrown which which would get absorbed by other error handling codeBefore:
After:
How did you verify your code works?
Manually, unfortunately.