Repository navigation
Response(asyncIterable): close the iterator with return(), never throw into it - #44862
Conversation
…ncel The cancel hook of an async-iterable body chose by the reason. A truthy reason went to iterator.throw?.(reason), a falsy one to iterator.return?.(). Until #41456, #41757 and #41894 no consumer sent a reason, so every cancel took the return() arm. Now a client that disconnects, the idle timeout of Bun.serve, a failed write to a pipe and cancel(reason) all send one. - An iterator with next() and return() and no throw() got no call at all. Its cleanup never ran. - A generator that catches what is thrown into it went on to a later `yield` and stayed there. Its `finally` never ran. The hook now closes the iterator with return() and no argument, whatever the reason is, the way `for await` leaves an iterator. node's Response(asyncIterable) and ReadableStream.from() do the same. A generator's `catch` no longer sees the reason of a cancel. closeIterator() also awaits what return() gives back the way `await` does. A thenable or a Promise subclass that starts its work in then(), like a Bun.sql query, now runs, also at the end of a body.
…nsumer fails The error tail of the pump called iterator.throw?.(error) for every failure, after it emptied the iterator slot. It had the same two holes as the cancel hook: an iterator with no throw() got no call, and a generator that catches stayed suspended at a later `yield`. A body that ends its own controller and yields again lands here, and so does a write to a pipe that fails while it is pending. The tail now follows `for await`: - The consumer failed (a refused chunk, a rejected write() or end()): the iterator is closed with return(). The body still rejects with the consumer's error. What return() itself throws still replaces it. - The iterator failed (next() threw or rejected, a malformed result): nothing more is called on it. A write() that fails at once came back as a rejected promise, and the pump dropped it and went on pulling for ever (Bun.write to /dev/full, a large write to a closed pipe). It now takes the error tail like a write that fails later. No code path calls iterator.throw() now. A generator's `catch` no longer sees the error of a failed write, and an iterator with throw() and no return() gets no call.
|
Status Reproduced with a
PR: #44862 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (13)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughAsync-iterable stream cleanup now calls ChangesAsync-Iterable Stream Cleanup
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Async-iterable response bodies are now closed with return() instead of having errors thrown into them. This matches for-await semantics and comes with broad test coverage. No concrete defect was identified, so the change appears ready to merge. Behavior on non-Linux platforms and on HTTP/2 and HTTP/3 was not exercised. 🚥 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 findings, I also checked the C++ side of the change: the reaction-handler macro in JSStreamsRuntime.h matches the JSC_DEFINE_HOST_FUNCTION set (NextRejected added, CancelRejected removed, no dangling references); closeIterator clears the Iterator field before invoking return(), so a cancel re-entered from inside return() or from a pending next() rejection finds nothing to call twice; and every throwing site in the pump (next getter/call, non-object result, done/value getters, sync-rejected next()) now abandons the iterator before the error tail, so return() is never invoked on an iterator that already failed. Since this reverses the cancel(reason)->throw() semantics of several unreleased PRs, a maintainer should still weigh the behavioral decision.
Extended reasoning...
The change rewrites the teardown of Bun's C++ async-iterable ReadableStream source (BunAsyncIterableSource.cpp) so cancel and the consumer-error tail always call iterator.return() instead of throw(), adds a promiseResolvedWithFast helper in WebStreamsMisc.cpp, removes one reaction handler and adds another, and ships a large test matrix plus a docs paragraph. It touches no auth, crypto, or input-parsing surface. The two inline findings are test-hygiene nits, but the change is a deliberate semantic reversal of recent unreleased behavior in a GC-sensitive streams path, so a human should confirm the policy and the exception-path handling rather than this review approving on its own.
| socket.on("error", seen.reject); | ||
| socket.on("close", () => seen.resolve()); |
There was a problem hiding this comment.
🟡 nit (optional): when the server ends or drops the response before "chunk" arrives, the new Bun.serve cases hang to the test timeout instead of failing with a reason. connectAndRead at test/js/bun/http/async-iterator-stream.test.ts:449 resolves seen on the socket's close event, so an early close returns a dead socket and the caller then waits forever on aborted.promise (:486) or on settled(shape). Fix: make every failure signal reject the awaited promise, so close before until was seen rejects with what was received (e.g. socket.on("close", () => seen.reject(new Error("closed before " + until + ": " + raw)))), keeping the resolve only on the data path at :446.
Why this was flagged
connectAndRead at test/js/bun/http/async-iterator-stream.test.ts:439-452 is the raw client for the 15 "when the client hangs up" cases (:474) and 5 "when server.stop(true)" cases (:491) plus the "after the client hung up" cases (:722 area). It resolves seen from the close handler at :449. If the server finishes or aborts the response before the body's first "chunk" is seen (a body that errors, a connection reset, a regression that ends the stream early), seen resolves anyway and the function returns a closed socket. The test then calls socket.destroy() on a dead socket and awaits aborted.promise at :486, which never resolves because req.signal only aborts on a client-side hang-up, so the case dies on the harness timeout with no message about what the server sent. The repository review rules under "Await conditions, wire failures to reject" ask that close/error events reject the awaited promise so the failure is diagnosable; only the error event does so here (:448).
Verification: The server ends or drops the response before the first "chunk" reaches the raw client. connectAndRead at test/js/bun/http/async-iterator-stream.test.ts:449 does socket.on("close", () => seen.resolve()), so an early close resolves seen like success. Callers then await aborted.promise (:486), resolved only from req.signal's abort, so the test sits until the harness timeout.
| }); | ||
| }); | ||
|
|
||
| test.concurrent("when server.stop(true) closes the connection", async () => { | ||
| const shape = make(); | ||
| const server = Bun.serve({ port: 0, fetch: () => new Response(shape.body) }); | ||
| const socket = await connectAndRead(server, "chunk"); | ||
| try { | ||
| await server.stop(true); | ||
| expect(await settled(shape)).toEqual(shape.expected); |
There was a problem hiding this comment.
🟡 nit (optional): a server leaks into later tests when this case fails before its try block. The server at test/js/bun/http/async-iterator-stream.test.ts:493 is a plain const, and connectAndRead at :491 runs before the try/finally that stops it, so a rejected connect leaves the listener open in a concurrent describe. Fix: declare the server with using (or move the connect inside the try) so it is stopped on every exit, as the sibling tests at :474 do.
Why this was flagged
The "when server.stop(true) closes the connection" test at test/js/bun/http/async-iterator-stream.test.ts:488 creates const server = Bun.serve(...) at :493 without using. await connectAndRead(server, "chunk") at :491 runs before the try { await server.stop(true) ... } finally { socket.destroy() } block at :492-497. If the connect or the read rejects (socket error wired to seen.reject at :452), the test fails with the server still listening, and nothing stops it. The surrounding describe.each is concurrent, so the leaked listener persists into later tests in the same worker; the base branch has no such test. test/CLAUDE.md and the review guidance ask for using or try/finally registered before assertions.
Verification: nit. Triggering condition: connectAndRead rejects or server.stop(true) throws. The server at test/js/bun/http/async-iterator-stream.test.ts:493 has no using, and the connect at :494 runs before the try/finally at :495-500, whose finally only destroys the socket, so a rejected connect leaves the listener open for the rest of the concurrent file.
2 participants
Problem
new Response(asyncIterable)leaves, the iterator can stay open for good.Bun.serve, the client hangs up:return() calls: 0; listeners left on the emitter: 5. 1.4.2 returns all five.reason ? iterator.throw?.(reason) : iterator.return?.()(src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp:462). The error tail callsiterator.throw?.(error)(:184). Since subprocess: surface stdio read and write errors instead of dropping them #41456, Direct streams read from JS: own the buffer, signal backpressure, run cancel() #41757, Bun.serve direct streams: flush on controller.close(), end the response once, call cancel() only on a client abort #41894 and HTMLRewriter: cancel a still-open JS stream input when the rewrite fails or its output is cancelled #42130 (none released) consumers send a reason.throw()gets no call. A generator that catches the throw never runsfinally.Fix
return(), the rule of Call cancel on ReadableStream when Bun.serve() response is aborted #13687 anddocs/guides/http/sse.mdx:19.for await: a failed write callsreturn(), a failed iterator gets no call. A write that fails at once takes this path too.return(). node andReadableStream.from()never callthrow().Background
ReadableStreampumped fromiterator.next(). The cancel hook runs when the consumer leaves, the error tail when the pump fails.return()only wherethrow()is missing: a generator that catches stays open. A reason-less close atstreams.rs:2015missesFileSink::on_close.Downsides
cancel(reason)no longer throws into a generator. Released change:catchno longer sees a failed write, and an iterator with onlythrow()gets no call.yieldagain.Notes
Who sends a reason (all end in the same cancel hook)
HTTPServerWritable::abort(streams.rs:2015): client hang-up, idle timeout,server.stop(true)AbortErrorFileSink::on_close: a failed write onBun.spawnstdin orBun.writeto a pipe or fileAbortErrorreader.cancel(reason),pipeToerror or abort,tee()/clone(),Readable.fromWeb().destroy(err),fetch()abortThe producers are not changed. Direct and default streams still get the reason in
cancel(reason).Rulings this follows
docs/guides/http/sse.mdx:19: "If the client disconnects, the generator'sfinallyblock runs so you can clean up."Responsebodies" under "Not changed".What it reverses
cancel(reason)throws the reason into the generator (asBun.servealready does on abort)". At that commitBun.servecalledcancel(undefined), which returned the generator. Its testcancel(reason) on an async generator body throws the reason into the generatornow expectsfinallyand nocatch.async generator body: client abort between yields throws the connection-closed error into the generator once, then finallynow expects["finally"], which is what 1.4.2 gives.iterator.throw?.(error)since 1.0.27. No existing test observed it.return()itself throws still replaces the body's error (webstreams: propagate exceptions through helpers, convert only at boundaries #39913). A rejectedreturn()promise does not.Results (debug ASAN build unless a build is named)
Bun.writeto/dev/full, 64 KiB chunksENOSPCafter 1 pull, body closednode v26.3.0 returns the iterator in 12 of 12 ways and never calls
throw().New tests on release builds of main and of this PR:
test/js/web/fetch/body-async-iterator.test.tstest/js/bun/http/async-iterator-stream.test.tstest/js/workerd/html-rewriter.test.js(new rows)test/js/bun/spawn/spawn-stdin-readable-stream.test.ts(new rows)test/js/bun/io/bun-write.test.js(new rows)test/js/bun/http/serve-direct-readable-stream.test.ts(rewritten pin)The 5 new cells that pass on main are the
reader.cancel()controls with no reason. Each cell runs five body shapes fromtest/js/web/streams/async-iterable-body-shapes.ts: an iterator withnext()andreturn(), one withthrow()too, a generator with try/catch/finally, a generator whose loop catches, and a nodeReadablewithautoDestroy: false.The two commits
/dev/fullwith 64 KiB chunks still pulls for ever.Still open, for a maintainer
return()at ayield. One that waits in anawaitis returned when thatawaitsettles (the same on 1.4.2). The docs paragraph says so and showsreq.signal.write()threw once the client left. Since Bun.serve direct streams: flush on controller.close(), end the response once, call cancel() only on a client abort #41894write()returns 0. The question is which of the two a write through the yielded controller must do after the consumer left.return()that throws synchronously whileBun.serveaborts the body is an uncaught exception, as on 1.4.2.catch(thrown: outOfMemory). They need afinallyflag so that neither PR turns main red.fs.closeSync(0)does nothing there. HTTPS, HTTP/2 and HTTP/3 share the abort path by reading, and only HTTP/1.1 was run.Measurements (release builds of main bd599f5 and of this PR)
size): 88,771,341 to 88,770,061 B (-1,280 B). Reaction handlers per global: 98 to 98.heapStatsslope, GC off, 2,000 to 6,000 chunks, 3 runs): 3.000 to 3.000 onBun.serveand.bytes().body.cancel(err): an iterator withthrow()Promise 4 to 3. An iterator withreturn()only Promise 2 to 3, becausereturn()is now called..text()(N=1000): a generator that throws 13 to 9 cells and 4 to 3 microtask turns. A refused chunk with a generator 14 to 15 cells and 6 to 7 turns. A refused chunk with{next, return}9 to 13 cells, becausereturn()is now called.return()gives a plain object and not a promise: 10 to 12 cells. The result is now awaited, so that a thenable or a lazyBun.sqlpromise runs.llvm-objdump): the four iteration-protocol exception checks in the pump go from 3 instructions to 2. Awrite()that returns a promise gets one more status compare.perfhardware counters fail in the build VM and valgrind could not be installed.Suites run on the debug ASAN build: the six files above,
streams.test.js,body.test.ts,body-stream.test.ts,fetch-args.test.ts,serve-stream-body-error.test.ts, and the two main files withBUN_JSC_validateExceptionChecks=1. In a CPU-capped container the subprocess tests ofserve-direct-readable-stream,spawn-stdin-readable-stream,bun-write,streamsandserve-stream-body-errortime out at the default concurrency. All pass with--max-concurrency 4 --timeout 60000. Two older chunk-count tests inasync-iterator-stream.test.ts(async generator function,[Symbol.asyncIterator]) fail in 2 of 6 debug runs with the new tests filtered out.Self-review, the one concern not taken: deleting the
lastResortoverload ofatStreamsBoundary. The error tail still uses it, because whatreturn()throws still replaces the error.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/workerd/html-rewriter.test.js, test/js/bun/spawn/spawn-stdin-readable-stream.test.ts, test/js/bun/io/bun-write.test.js, test/js/bun/http/serve-direct-readable-stream.test.ts