Conversation
Collaborator
Author
|
Status: reproduced on 1.4.2 and on main (29d9638).
This PR is a draft. The second commit makes one working handler shape fail (see Downsides), so it waits for a maintainer's decision on the premise that it shares with #41585. This is the one PR for this work. #43972 is closed, and its commit is the first commit here. |
robobun
force-pushed
the
robobun/e4fa84d4/head-locked-stream-error
branch
from
September 25, 2026 11:10
1e21973 to
9b940dc
Compare
A locked stream belongs to its lock holder: a reader the handler took, or the server's own sink for another Response around the same stream. Two places ended such a stream. cancel_unread_body cancels the body stream of a Response the server does not transmit: a HEAD response, a 101/204/205/304 response, and a handler result the server drops. It now cancels only a stream that is not locked. ReadableStream.prototype.cancel rejects on a locked stream for the same reason. GET refuses a locked stream body with ERR_STREAM_CANNOT_PIPE. The refusal left the stream attached to the Response, so the teardown of the refused request found it and ended it. The refusal now detaches the stream. It also no longer calls unprotect() on the stream, which no protect() matched.
… does GET refuses a Response whose body stream a reader already holds: it calls error() with ERR_STREAM_CANNOT_PIPE. The HEAD renderer had no such check and answered the handler's status with transfer-encoding: chunked. can_send_body_stream is the one place both renderers ask whether a stream body can be sent. refuse_body_stream builds the error() argument and sets the state a refusal leaves: the body is used and the Response lets go of the stream without a cancel. The HEAD arm asks before it writes any header. It hands the stream it resolved to cancel_unlocked_body, so a HEAD with a sendable stream resolves the stream once and asks for the lock once. The shared parts are free functions that are not inlined, so the eight RequestContext monomorphizations share one copy.
robobun
force-pushed
the
robobun/e4fa84d4/head-locked-stream-error
branch
from
September 25, 2026 12:17
9b940dc to
b995e28
Compare
Collaborator
Author
|
Updated 5:46 AM PT - Sep 25th, 2026
✅ @robobun, your commit b995e28d43081c5c17c7c1a3b9756c4a35cab6e9 passed in 🧪 To try this PR locally: bunx bun-pr 43973That installs a local version of the PR into your bun-43973 --bun |
This branch has not been deployed
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.
Draft: a maintainer must decide on the first Downsides bullet. The base is #41585.
Problem
Bun.serveends a body stream that another reader holds. The producer's nextenqueue()throwsInvalid state: Controller is already closed. Regression since 1.4.0.error()withERR_STREAM_CANNOT_PIPE. HEAD answers 200.cancel_unread_body(src/runtime/server/RequestContext.rs:740) and the HEAD renderer (:2662) have no lock check. GET's refusal (:3146) leaves the stream where teardown finds it.Fix
can_send_body_stream, before they write a header.refuse_body_streamdetaches the refused stream.cancel_unread_bodyskips a locked stream:ReadableStream.prototype.cancel()rejects on one.serve-reused-response.test.tsandserve-pending-promise-abort-leak.test.tsfail at the base. Self-reviewed: 6 and 7 concerns raised for the two commits, all addressed.Background
ReadableStream__cancelWithReason.fetch()cancels a body that it locked itself, so the rule is the server's.Downsides
ReadableStream__isLockedcall (HEAD: 12 more instructions). GET does not change. Release function bytes: +81 B and +323 B for the two commits.Notes
Stack. The base of this PR is #41585 (HEAD for file bodies and for used or errored bodies). This PR has two commits. The first commit stops the server from ending a stream that another reader holds. It was #43972, and it also applies to main without #41585. The second commit adds the HEAD refusal. I rebase onto main when #41585 merges.
Reproduction, first commit. Same result on 1.4.2 and on main (29d9638):
Which release broke what (released binaries, Linux x64):
(1) 1.3.14 does not refuse the second Response. It answers 200 and the first download stalls.
The callers. The callers of
cancel_unread_bodyget the rule with no change of their own:do_render_null_body_status_corked(101/103/204/205/304) anddiscard_response_body(a handler result that arrives after the client aborted, afterserver.stop(true), after an upgrade, or when the connection closed during dispatch). TheLockedarm ofdo_render_head_responseaskscan_send_body_streamfirst, so it callscancel_unlocked_bodydirectly.GET's refusal. GET answers a locked stream body through
error()withERR_STREAM_CANNOT_PIPE. With noerror()handler, or one that returns nothing, the request keeps the refused Response until it ends.release_body_stream(at finalize) andon_abortlook the stream up through the Response and end it. A handler that holdsupstream.body.getReader()and returnsupstreamread 1 of 6 chunks. It now reads 6. With anerror()handler that returns a Response, main already reads 6, so the tests use the default 500 and run in a child process.The refusal also no longer calls
stream.value.unprotect(). Noprotect()of a stream value exists insrc/runtime/server/orsrc/runtime/webcore/, so the call had nothing to balance.Reproduction, second commit. Same output on 1.4.2 and on main (29d9638):
The decision. It applies to the second commit only. The first commit changes no status.
The server sees only the Response that the handler returned for HEAD. The second commit assumes that GET gets a Response in the same state. That holds for a shared or cached stream. It does not hold for a handler that treats HEAD in its own way:
The handler gets the old result when it returns
new Response(null, response)for HEAD.Nobody reported the HEAD result as a bug. It came from a review note on #41585.
What the tests cover, first commit. The lock comes in three kinds, and the cross-request tests run each: a reader (
ReadableStream), the sink of atype: "direct"stream, and the native pipe of afetch()body. Each kind counts thecancel()calls of its source. For thefetch()kind that is the stream of the upstream server. With the guard alone and no detach, the two GET tests fail and the other 13 pass, so each change has a test that needs it.What the tests cover, second commit. 8 locked shapes (
getReader()on start, pull andtype: "direct"sources, with a Content-Length header,stream.getReader()without.body,tee(),await response.text(), another Response around the same stream) x a Response, a fulfilled promise and a pending promise. Each test runs GET and HEAD, pins what GET does (status 500,content-length: 7, oneerror()call, zerosource.cancel()calls, the lock holder reads the whole stream), and asserts that HEAD gives the same result with no body. So the tests fail when GET changes and HEAD does not follow. More tests: an any-method route, a HEAD route and a HEAD derived from a GET route, HTTP/2, the same Response returned twice (ERR_STREAM_CANNOT_PIPEthenERR_BODY_ALREADY_USEDfor every order of GET and HEAD), and the default 500 with one child process per method and an exact count of error reports.With
src/at the base, 34 of 49 tests fail inserve-reused-response.test.tsand 12 of 39 inserve-pending-promise-abort-leak.test.ts. 15 of them belong to the first commit.serve.test.tshas no new test on purpose. Two of its tests fail in my environment with and without the change, so a run of that file cannot show a pass.Measurements, first commit. The base for these numbers is main at 29d9638. Release builds unless a line says otherwise.
source.cancel()calls on a stream that somebody else holds:Stream FFI calls per request (gdb breakpoint hit counts over 100 requests from curl, debug build, the N=0 control is 0):
Instructions that each function executes per call, callees stepped over (gdb
nexti, HTTP/1 production copy): GET with a string body 347 -> 347 and with a stream body 348 -> 348, summed overon_response,protect_for_body_and_render,render,do_render_with_bodyandrender_bytes. HEAD with a stream body 185 -> 185 inon_responseanddo_render_head_response.Release size (
llvm-nm --print-size, summed over distinct addresses, andsize):cancel_unread_body795 -> 836 B at one address,do_render_with_body+5 B in each of 8 copies..textstays at 80,856,413 B and the stripped binary at 80,995,912 B.JS objects retained after
Bun.gc(true)are equal with and without the change: after 1000 fresh-stream HEADsReadableStream0 -> 2 andprotectedObjectCount4 -> 4.Wire capture: a raw-socket client sent GET and HEAD for 18 body shapes. With the Date header removed, all 36 response heads and bodies are byte-identical.
A handler that releases or cancels its reader before it returns leaves 0 producers open after 300 requests (HEAD, and GET with a 204). Only a reader that the handler abandons gives the 300 of Downsides.
Measurements, second commit. The base for these numbers is #41585 plus the first commit. Release builds unless a line says otherwise.
Release size (
llvm-nm --print-size, summed over distinct addresses, andsize):An earlier shape kept
cancel_unlocked_bodyas a method of the genericimpl. LLVM then inlined the smallcancel_unread_bodyinto all 8 copies ofdo_render_null_body_status_corked, and the total was +4041 B. The shared parts are free#[inline(never)]functions for that reason. Thegeneric_body_not_genericcount of mordant forRequestContext.rsis 7 before and after each commit.Instructions that each function executes per call, callees stepped over (gdb
nexti, HTTP/1 production copy):Stream FFI calls per request. In a release build a HEAD with a stream body makes one lock check and one stream lookup, as with the first commit alone:
do_render_head_responsecallsReadableStream__isLockedonce andcancel_unlocked_bodydoes not call it (disassembly). A breakpoint count is not possible there, because LTO inlines the callee. In a debug build thedebug_assert!incancel_unlocked_bodyadds oneisLockedcall per HEAD, 204 or 304 response with a stream body (3 -> 4, gdb hit counts over 100 requests).taggedStreamandcancelWithReasondo not change. GET and string bodies do not change.Write and send syscalls per HEAD response (gdb
catch syscall, 100 requests): fresh stream 1 -> 1, locked stream 1 -> 1. Theerror()Response of a refused HEAD leaves in one write.Wire capture: a raw-socket client sent GET and HEAD for 18 body shapes. With the Date header removed, the base and this PR give identical response heads and bodies on 18 of 18 GET rows and 11 of 18 HEAD rows. The 7 HEAD rows that differ are the locked-stream rows:
200withtransfer-encoding: chunkedbecomes500withcontent-length: 7.Removed after review. An earlier shape had one more commit: a microtask checkpoint in the HEAD branch of
render(), which onlyerror()Responses reach. It made HEAD match GET when a microtask fromerror()locks the stream. It also copied two GET defects to HEAD: a heldfetch()body ended after 1 of 4 chunks, and an unread stream was not cancelled. #43971 tracks that window for both methods. Until then HEAD differs from GET in that one case: GET answers the default 500, HEAD answers the status of theerror()Response.Other differences between HEAD and GET for a stream body, not in this PR.
read(),releaseLock()): GET sends the rest with a 200, HEAD answers 200 chunked. Bun.serve: reject disturbed ReadableStream response bodies with ERR_BODY_ALREADY_USED #36110 adds anis_disturbedrule to GET's arm. After this PR that rule is one line incan_send_body_streamand reaches both methods.start(): GET closes the connection and does not callerror(), whichserve-stream-body-error.test.tspins. HEAD never starts the stream, so it answers 200.fetch()Response with an unawaitedtext()ends the process with SIGSEGV #43969: aLockedbody with no stream and a pendingtext()ends the process on GET. HEAD answers 200.fetch()Response holds the upstream request open, andfetch()stops after 256 requests #43970: HEAD of afetch()Response that the handler returns untouched leaves the upstream request open. That Response has no stream yet, socancel_unread_bodyhas nothing to cancel.FetchTasklet.rsholds a second copy of theERR_STREAM_CANNOT_PIPEconstruction.do_render_head_responsestays a hand-written mirror. Bun.serve: resolve a file body for HEAD the same way as for GET #41585 lists its retirement as a follow-up.Also not in this PR.
error()Response ends a stream that another reader holds #43971:on_abortandrelease_body_streamdo not know if the request attached to the stream. After this PR one window is left: a request that ends in the microtask checkpoint of anerror()Response. The issue has the measurements for a lock check at those two sites.cancel_unread_bodyinto a free function. Both PRs change that function.render_metadataskips the fallbackContent-Typewith a pointer comparison againstMimeType::OTHER.OTHERis aconst, so it has no single address. A release build merges the copies and omits the header for a stream body. A debug build does not, and sendscontent-type: application/octet-stream.Other suites on the debug build:
bun-serve-file.test.ts,serve-direct-readable-stream.test.ts,bun-server.test.ts,serve-error-handler-stream.test.ts,serve-stream-body-error.test.ts,serve-http2.test.ts,serve-http3.test.ts,bun-serve-static.test.tsandbun-serve-routes.test.tspass.serve.test.ts: 323 pass, 2 fail with and without the change in my environment (the root-range port test, because the container runs as root, and the/bun:infoloopback test, because an egress proxy answers the non-loopback request).fetch.stream.test.ts: 6 or 7 tests (Content-Length response works (multiple parts)) time out at 5 s in a run of the whole file, with and without the change, on a host with a load average of 90. They pass when they run alone.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file