Repository navigation
Conversation
The pull algorithm of a default tee let a throw from its read of the source leave through the branch controller. The controller had set [[pulling]] already, so that branch never pulled again and its consumer never settled. The error went to whichever frame made the pull: the runner reported it as uncaught, or a read() on a branch threw it synchronously. A type: "direct" source can throw from that read. Its close() hook runs after pull() returns when close() or end() was called inside pull(). The pull algorithm is now the boundary for that throw and never throws. One read feeds both branches, so both branches fail with the error and the cancel promise settles. If the source already gave the branches their outcome (it closed them, or it failed with its own error), no branch is left to take the error, and it is reported to the runner.
|
Status for 7d348f5. How it was reproduced: the script below prints process.on("uncaughtException", e => console.log("uncaught:", e.message));
const rs = new ReadableStream({
type: "direct",
pull(c) {
c.write("a");
c.close();
},
close() {
throw new Error("close hook");
},
});
const res = new Response(rs);
res.clone();
const pending = new Promise(r => setTimeout(() => r("PENDING"), 500));
console.log(await Promise.race([res.text().then(v => "resolved:" + v, e => "rejected:" + e.message), pending]));
process.exit(0);With PR: #43956 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughDefault stream teeing now routes synchronous source-read errors through shared branch-erroring logic. Tests cover throwing direct-source close hooks across tees, clones, nested tees, cancellation, and cases where branches already received the source outcome. ChangesDefault stream tee error delivery
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change appears mergeable after normal checks; no remaining issue is established that would leave tee consumers pending. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also diffed the extracted defaultTeeErrorBranches against the old inline body of defaultTeeReaderClosedRejected — same error-then-resolve sequence and cancel polarity, so that refactor is behavior-preserving; and the atStreamsBoundary deliver runs no user JS before deciding, so a throwing deliver cannot double-report.
Extended reasoning...
The change touches only the default-tee pull path in ReadableStreamOperations.cpp and its tests, no security-sensitive surface. One confirmed finding (behavior change for programs that caught the close hook's error on the base branch) is posted inline, so a human look is already signalled.
| bool open = (controller1 && readableStreamDefaultControllerCanCloseOrEnqueue(controller1)) || (controller2 && readableStreamDefaultControllerCanCloseOrEnqueue(controller2)); | ||
| if (!open || teeState->stream()->m_state == ReadableStreamState::Errored) | ||
| RELEASE_AND_RETURN(scope, Bun__reportUnhandledError(globalObject, JSValue::encode(error))); |
There was a problem hiding this comment.
🟡 (optional) Programs that caught the close hook's error from a branch read on the base branch now get an uncaught exception and exit code 1 instead. ReadableStreamOperations.cpp:1131 routes the throw to Bun__reportUnhandledError whenever the branches are already closed, e.g. the source closes with no final chunk on a later pull. On base that throw left the pull through read()'s scope at JSReadableStreamDefaultReader.cpp:725/737, so await reader.read() rejected and a try/catch handled it. Fix: give the error to the caller that made the read (reject that branch's read, or error the branches) instead of the process-level reporter; keep the reporter only when no consumer exists at all.
Why this was flagged
Trigger: a direct source teed (stream.tee(), Response.clone(), Request.clone()) whose close() hook throws after the source closed without a final chunk, e.g. the PR's own secondPullClosesOnly shape (test/js/web/streams/streams.test.js:4376): pull 1 writes "a", pull 2 only calls close(). The second pull is made from the branch's reader.read() (JSReadableStreamDefaultReader.cpp:724-742), which on base returned the hook's error as a synchronous throw or rejection of read() that user code could catch. The new deliver at ReadableStreamOperations.cpp:1129-1131 computes open=false because closeSteps (JSReadRequest.cpp:142-153) already closed both branches, and calls Bun__reportUnhandledError. uncaught_exception (VirtualMachine.rs:2143-2162) with no process handler prints the error and sets exit_handler.exit_code = 1; read() resolves normally. The dismissal took the PR's table (87 cells report on main) as proof the population is unchanged, but the PR itself lists 3 cells where main threw from read(); those move from catchable to uncatchable with a nonzero exit. Remedy: reject the read that…
Verification: nit — acknowledged in diff: the description's table admits "read() throws synchronously: main 3 → this PR 0" and the added subprocess test pins secondPullClosesOnly: { a: { ok: "a" }, b: { ok: "a" }, uncaught: ["close hook"] } (test/js/web/streams/streams.test.js, "an error that no branch can take is reported…"), but the Downsides claim that these cells are "reported as uncaught, as…
There was a problem hiding this comment.
This finding is correct. I measured it on release builds of main and of this PR.
Source: the first pull() writes "a" and flushes, the second pull() only calls close(), and the hook throws.
| main | this PR | |
|---|---|---|
| Reader waits (a microtask makes the pull) | both branches get "a", the error is reported as uncaught | the same |
Reader is late (its read() makes the pull) |
that read() throws close hook synchronously, branch a never gets "a", nothing is reported |
both branches get "a", the error is reported as uncaught |
Late reader with a try/catch, no process handler |
catches the error, 0 chunks, exit code 0 | no error in the loop, 1 chunk, exit code 1 |
Two corrections to the numbers:
- Of the 3 measured cells where main threw from
read(), 1 moves to the reporter. In the other 2 the closingpull()writes a final chunk. Both branches now reject with the hook's error, so both consumers can catch it. - The Downsides section said "as on main" for all 40 cells. That was wrong for the late reader. The PR body now states the change.
Why the code reports and does not rethrow: on main a caller can catch the error on one path only. On that path read() throws synchronously and the branch loses the chunk that it dequeued. The frame that gets the error depends on timing.
I did not change the code. The PR body lists three options under "Open choice for a maintainer". I will build the one that a maintainer prefers.
This branch has not been deployed
1 participant
Problem
type: "direct"source has aclose()hook that throws. Aftertee(),Response.clone()orRequest.clone(), a consumer of the first branch never settles. The error is reported as uncaught.defaultTeePullAlgorithm(ReadableStreamOperations.cpp:1105) lets the throw leave through the branch controller, which already setm_pulling. That branch never pulls again.Fix
textDecodePullAlgorithm. It never throws.deliverDirectErrordoes.test/js/web/streams/streams.test.js(32 new tests, 0 pass on main) and 11 more suites. Self-reviewed: 12 concerns raised, 11 addressed, 1 in part.Background
close(reason)hook. It runs afterpull()returns whenpull()calledclose().promiseFromStepsalone: 4 cells hang and the branches disagree in 25 of 110. No design review was completed before the code.Downsides
read(). A caller could catch it, but lost a chunk. Now the caller gets the chunk and cannot catch the error.buntext grows by 512 bytes. A pull that does not throw makes no new call. Instructions are not counted:perfandvalgrindare missing.Notes
Repro
Regression window. Release binaries: 1.3.14 and 1.4.0 resolve, 1.4.1 and 1.4.2 never settle. #39913 is in 1.4.1. It made the hook's throw propagate and gave
textDecodePullAlgorithmits boundary. The two tee pull algorithms did not get one. I did not bisect commit by commit.Measurements. Debug builds of main (73df7bb) with one file changed. One probe, 11 source shapes x 12 consumers = 132 cells, 110 of them with a tee. The hook throws in every cell.
promiseFromStepsaloneread()throws synchronouslyread(), or has branches that disagree.promiseFromStepsalone reports 0 uncaught because it drops the error in those cells.read().Where the error goes
close()orend()insidepull()with a final chunkclose()with no final chunkpull()throws,close(error)A branch that still holds a queued chunk counts as closed. It keeps the chunk.
Open choice for a maintainer: the error when no branch can take it. The code review on this PR found that this PR takes away a way to catch the error. It is correct. Measured on release builds, with a source whose second
pull()closes with no final chunk:read()makes the pull)read()throwsclose hooksynchronously, branchanever gets "a", nothing is reportedtry/catch, no process handlerThe same holds when a later
pull()throws: on main a lateread()throws the hook's error, and with this PR the hook's error is reported and the branches reject with the error ofpull().The third option is not built. It moves the close of the branches after the hook for a hook that does not throw. The self-review asked for the first option. The code review asks for the second or third. I kept the first and will change it if a maintainer prefers another.
Why report and not drop. The review thread of #39913 states the rule: "a hook throwing after the stream already closed is reported rather than dropped, without double-reporting errors that did error the stream" (#39913 (comment)). The tee's pull algorithm runs in the frame of whoever pulled, so a rethrow does not reach the runner reliably. On main a
read()on a branch throws the hook's error synchronously and loses a queued chunk. So the deliver step callsBun__reportUnhandledError.Sites that have the same shape and are not changed
byteTeePullAlgorithmTypeError: Cannot close a ReadableByteStreamController while a BYOB read request is partially filled. The other branch then stays pending until one more read. 1.4.0 does the same. The throw comes from the tee's close step, not from the source, so it needs its own fix.Body.textStream()close()with no final chunkbytes()andarrayBuffer()with no teeSelf-review. It raised 12 concerns and changed the fix twice.
pull()had thrown too. The source's own error was then reported nowhere. Now the branches keep the source's error.read()on a branch then threw synchronously. Now the error goes to the runner.Tests
close hook.cancel()of the other branch settles.src/at main: 0 of 32 pass (debug build). On the 1.4.2 release: 0 of 32 pass.BUN_JSC_validateExceptionChecks=1: 0 reports on the new tests, the probe, and the 63 tee tests of the file. A deliberate violation makes the validator abort, so it is live.Suites run on the fix (debug build).
streams.test.js656,body-stream9086,body774,body-clone85,compression73,response23,body-mixin-errors13,sync-pull-fast-path7,body-async-iterator6,native-source-onclose-leak4,readable-stream-terminal-barrier-release2,pipeTo-shutdown-gc1, and 7 node compat stream files. All pass.Size. Two LTO release builds that differ in one file.
bun: text 80,676,462 to 80,676,974 (+512), data and bss +0, file size 80,844,320 in both.defaultTeePullAlgorithm: 469 to 553 bytes. Its one new call is the deliver step, out of line, on the throw path only.Found on the way, not changed here
response.test.ts"handle stack overflow" takes 4.4 to 4.5 s of its 5 s limit on a debug build, on main and on this PR alike. It timed out once under host load.new Response(rs).text()stays pending after one syncpull()that writes and flushes with noclose(). This happens with no hook and no tee. It can be intended.close(new Error("reason"))insidepull()with a hook that throws: the consumer rejects with the hook's error andreasonis reported as an unhandled rejection.Rebase. #41472 adds tests to the same describe block of
streams.test.js. The PR that lands second needs a manual rebase of that file.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/streams/streams.test.js