Repository navigation
Conversation
… dropped `cancel_unread_body` runs for the Response of a request that the server does not answer: the client left, the idle timer closed the connection, the request became a WebSocket, or the server stopped. It stored `Body::Value::Used` for every kind of body. A Response without a body that the handler keeps was then refused with ERR_BODY_ALREADY_USED on every later request. A null or empty body has nothing to release, so it now stays as it is. A body with content and a stream are given up as before.
|
Status How I reproduced it: a handler returns one kept With this PR the later GETs answer 404 again and |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. Walkthrough
ChangesResponse body reuse
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The bodyless-response reuse change is mergeable after normal checks. The separately reported response-reuse limitations remain outside its scope. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the observable body state of a dropped Response in the Bun.serve lifecycle and the description names three open PRs that rewrite the same statement in cancel_unread_body, a human look to settle the ordering with those is still worthwhile.
What was reviewed:
- The guard at
src/runtime/server/RequestContext.rs:761—NullandEmptyown nothing theUsedstore would release, anddo_render_with_bodyrenders both as an empty body, so a kept bodiless Response answers again;Error, string, Blob and stream bodies still hit theUsedarm. - The other
Body::Value::Usedstores in the file (release_body_stream, the stream-resolve teardown, theLockedrender arm) only apply to streamed or rendered bodies, so no sibling site needs the same condition. - The new tests await the abort/close/open events, wire WebSocket
onerrorto reject, cover sync and async handlers plus abort, upgrade andstop(true), and keep a negative-contract case for bodies with content.
Extended reasoning...
The change is a six-line condition in cancel_unread_body in src/runtime/server/RequestContext.rs plus four new tests in test/js/bun/http/serve-reused-response.test.ts; it touches no security-sensitive surface. The guard restores pre-1.4.1 behavior for bodiless Responses and matches the Fetch spec, where bodyUsed is false for a null body. No CODEOWNERS entry covers the changed paths and the bug hunt ran dry. Deferring rather than approving because the PR alters user-visible server lifecycle semantics introduced by a recent fix (#41011) and the author lists three open PRs that rewrite the same statement, so a maintainer should decide merge order and whether the bodyUsed change is the intended contract.
Problem
Responsewithout a body, such asnew Response(null, { status: 404 }), and returns it for every request. After one request that the server does not answer, each later request gets 500ERR_BODY_ALREADY_USED: "Response body already used. A Response body can only be sent once; create a new Response for each request."cancel_unread_body(src/runtime/server/RequestContext.rs:760) storesBody::Value::Usedfor every kind of body, since Bun.serve: cancel the body stream of a Response the server will never transmit #41011 (1.4.1 and 1.4.2).Fix
cancel_unread_bodyleaves aNullorEmptybody as it is. It gives up every other body as before.Responseowns nothing that the store releases, and Fetch keeps itsbodyUsedfalse.test/js/bun/http/serve-reused-response.test.ts, 5 new tests, 4 fail on main. The fifth is a control for bodies with content. Alsoserve-pending-promise-abort-leak.test.ts.Background
Responseof a request it can no longer answer, and cancels a stream body.Usedalso releases a string,Blobor errored body (impl Drop for Value,Body.rs:1434). A GET refuses aUsedbody (Bun.serve: invoke error() when the returned Response body is already used #33118).Blobbody then keeps its bytes until GC.Downsides
bodyUsedof a body-lessResponseisfalse. Since 1.4.1 it wastrue.Responsestill sends its headers in the first answer only (since 2022).Notes
Where it reproduces. I ran two scripts from the report. In the first, the handler returns the kept
Responsebehindserver.upgrade(req). In the second, a client leaves by a close or a reset while the handler awaits. Both are right on 1.3.13. Both fail on 1.4.2 and on a release build of main (bd599f5).git tag --contains 936bf867aalists bun-v1.4.1 and bun-v1.4.2.With this change (debug build of this branch): both scripts pass. A script with a slow handler under the default idle timer passes. A fourth script drops a request in four ways over seven kept shapes:
server.upgrade(),idleTimeout: 1,server.timeout(req, 1), andserver.stop(true)with a new server. It has 20 of 28 broken cells on main and 0 of 28 here. That script compares the status line only.The test file. With
src/at main (620b50f): 12 pass, 4 fail. The 404 row and the empty string row answer 500 three times througherror(). All three kept objects saybodyUsed: true. With this change: 16 pass.The abort suite. I ran
serve-pending-promise-abort-leak.test.tstwice on each of two debug builds: main and this branch. Main: 27 pass and 3 fail, both times. This branch: 27 and 3, then 26 and 4. Every failure is the 5 s default timeout of a test that starts a debug subprocess. The one extra failure passes 3 of 3 alone on both builds.Not changed here.
Response: the first answer has the headers, later answers do not. It needs its own change.new Response("")whose.bodywas read answers 500 from the second request on main. The cause is theEmptyarm ofValue::to_readable_streamsince Keep body stream bookkeeping independent of the body's source #42116. It does not entercancel_unread_body.Responsewith a body. A dropped string, bytes or stream body is marked used as before (tested). ABlob, file or errored body takes the same arm (read from the code, not tested).Open PRs that rewrite the same statement. Each keeps the store for every body. Each needs the same condition if it lands after this one.
.discard().cancel(JSValue::UNDEFINED).cancel_unlocked_body.cancel_unread_bodyinto a free function.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file