Repository navigation
fetch: restore the one-shot gzip inflate for text() and arrayBuffer() - #43386
Conversation
A gzip response body that arrives whole usually does so before the caller has the Response. The receive mode is still Flowing then, so the 256 KB output budget from #43123 sends the body through zlib passes. Before that budget, one exact-size libdeflate call inflated it. FetchTasklet now starts in a new receive mode, Unclaimed: no consumer has attached yet. When a complete gzip body whose trailer size is above the 512 KB shared buffer and below 32 MB has not been touched by a decoder, the HTTP thread moves Unclaimed to Paused and decodes nothing. The consumer that attaches resumes the transport. A buffered consumer (text, arrayBuffer) then gets the one libdeflate call. A reader gets budgeted zlib passes as before. Nothing is decoded for a Response that nobody reads. The tests cover a buffered consumer and a reader, with the body arriving with the head, after the head, and after the consumer, over TLS and through a CONNECT proxy, for a Response nobody reads, and for a collected Response. The CONNECT proxy cases clear NO_PROXY and the proxy variables for the child and assert that the proxy saw one CONNECT.
|
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 (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughChangesThe change adds bounded exact-size gzip inflation, introduces an Gzip backpressure and inflation
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete merge-blocking behavior is established at the current head. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:09 PM PT - Sep 18th, 2026
✅ @robobun, your commit e98f60c7ff61a925817cef41791d5f85b430c54e passed in 🧪 To try this PR locally: bunx bun-pr 43386That installs a local version of the PR into your bun-43386 --bun |
No code changes. Each comment that spanned two or three lines now says only what the code cannot, in one line.
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 checked the is_libdeflate_fast_path_disabled change the description flags: decompress_bytes sets the flag itself on entry to the libdeflate block (src/http/InternalState.rs:313) and any zlib pass leaves a non-None decompressor, so a second pass over a final chunk cannot re-enter the exact-size path unless the first pass decoded nothing (the held/paused case, which is the intent). I also looked at the window between reading the cap and the Unclaimed -> Paused CAS: a consumer attaching there flips the mode first, the CAS fails, and the cap is re-read, so no body is left undecoded with a waiting consumer.
Extended reasoning...
A confirmed finding is posted inline, so this note only records what else was examined. The PR description explicitly asks a reviewer to check the relocated is_libdeflate_fast_path_disabled assignment in handle_response_body_from_multiple_packets (src/http/lib.rs:4712-4716); reading decompress_bytes confirms the flag is set at src/http/InternalState.rs:313 whenever the libdeflate block is entered, and every other decode path constructs a Decompressor, which the entry condition at line 307 also rejects, so the only way to reach libdeflate on a later final-chunk pass is a body that was never decoded, i.e. the hold. The cap-read-then-CAS ordering in process_received_body (src/http/lib.rs:4120-4136) was also traced: receive_all/receive_on_demand on the JS thread move the mode away from Unclaimed before scheduling the resume, so either the CAS loses and max_output is re-read under the new mode, or it wins and the consumer's later transition out of Paused triggers the resume that decodes the pending body.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/http/lib.rs— A streaming reader of a held gzip body gets one unbudgeted chunk of up to 32 MiB when the origin closes the connection first. finalize_body_on_eof at src/http/lib.rs:2148 decodes with usize::MAX. Through a CONNECT tunnel the socket is never paused (maybe_pause_receive bails at src/http/lib.rs:4146), so an origin keep-alive timeout reaches on_close while the whole compressed body is still undecoded and Paused. Fix: on EOF for a body that is complete and held, decode under the consumer's cap (decompress_output_cap) and leave the rest pending for the pump, instead of passing usize::MAX.Extended reasoning...
The finder called this pre-existing because InternalState.rs:257 is unchanged. It is, but the state that reaches it changes. On the base a held body already had 256 KB decoded and delivered; the hold now leaves the entire body undecoded (decompress_output_pending=true, nothing in decoded_body) at src/http/lib.rs:4131. Trigger: fetch through a proxy tunnel (or any path where the socket stays readable), the response body of 512 KB-32 MB decoded arrives whole, the caller obtains res.body but does not read it for longer than the origin's keep-alive idle time (commonly 5-15 s at CDNs). The origin closes; ProxyTunnel sees the close, on_close runs; in_progress is true, is_body_complete_on_close at InternalState.rs:257 returns true because decompress_output_pending && is_done. finalize_body_on_eof at InternalState.rs:275 calls process_body_buffer with usize::MAX; libdeflate inflates the full 32 MiB into decoded_body; progress_update at lib.rs:2153 hands the whole buffer to the FetchTasklet callback, which appends it to scheduled_response_buffer and the reader receives one 32 MiB chunk.…
Verification: pre-existing. Trigger: an h1 gzip body of 512 KiB-32 MiB decoded arrives whole while the socket stays readable (CONNECT tunnel:
maybe_pause_receivereturns early onself.proxy_tunnel.is_some()at src/http/lib.rs:4146; or the on_data/JS-pause race on a direct socket), and the origin closes before the consumer drains it. Mechanism verified: after the hold (src/http/lib.rs:4130-4132,…
|
Status (head e98f60c) How to reproduce, with a debug build: bun bd test test/js/web/fetch/fetch-backpressure.test.ts -t 'arrived whole'
Review state: an automated review of 3ca187d found the decode at the end of the transport, and the description and the four cases above answer it. An automated review of e98f60c reported no issues. No self-review of this diff has finished, so this PR has had no adversarial design review. Two risks that no reviewer has examined, checked by me as the author:
The description names the refactor that I would most like a second pair of eyes on ( |
Four cases for a body that the client holds undecoded when the origin ends the connection before a consumer attaches. On a direct socket the client sees the close once the consumer resumes it. Through a CONNECT tunnel the close arrives while the body is held, and the test waits for the client to close its side before the consumer attaches. A buffered consumer and a reader get the exact bytes in both.
|
Thanks for the review. The finding about the end of the transport is correct, and my description claimed too much.
The review says that a confirmed finding is posted inline. I see no inline comment from it on this PR. If one was intended, it did not arrive. The reviewed commit and the current head differ in comments and tests only. |
… its consumer main's #43386 held such a body in the HTTP client (Unclaimed -> Paused) until a consumer attached. Here the request ends and the body goes to the consumer as a HeldBody, so that hold is gone: - process_received_body decodes nothing of it while no consumer has attached. - HeldBody::decode inflates it in one exact-size libdeflate call for a consumer that takes the whole body. A reader gets budgeted zlib passes. - FetchTasklet decodes nothing of a held body until a consumer attaches. - Signals::hold_for_consumer is removed. The test of a collected Response with a held body expected the origin to see its connection close. The request has ended by then and the connection is back in the pool, so the test now counts the freed tasklets and the decode passes.
Problem
arrayBuffer()ortext().Response, so the mode is stillFlowing.decompress_bytes(src/http/InternalState.rs) budgetsFlowingat 256 KB and runs zlib passes.Fix
FetchTaskletstarts in a new receive mode,Unclaimed: no consumer has attached. It is demand-driven, likeFlowing.process_received_body(src/http/lib.rs) movesUnclaimed -> Pausedand decodes nothing. The consumer that attaches resumes the transport.BufferAllgets the libdeflate call, and a reader gets budgeted zlib passes.Unclaimeddecides who was first. A held body is decoded only when a consumer attaches or its connection ends (Notes).test/js/web/fetch/fetch-backpressure.test.ts. The debug-log case fails on main with two zlib passes. The whole file passes.Background
BodyReceiveMode(src/http/Signals.rs) is the receive backpressure for a body handed to JS. UnderFlowingandPausedthe client decodes at most 256 KB per pass.BufferAll(.text(),.arrayBuffer()) never pauses.Notes
#43169 rewrites the same function. Please read this before choosing a merge order. #43169 (open) rewrites
process_received_bodyand givesdecompress_output_capa parameter, so the two PRs conflict there. It does not fix this regression. I built itssrc/at 31132d6 under this PR's tests: the debug-log case still fails, with the libdeflate attempt followed by a zlib pass over the body. So both changes are needed, and the one that lands second has to carry the hold through the other'sprocess_received_body. One more case of this PR,held, and its Response is collected: the fetch is aborted, times out after 5 s on that build, in 3 of 3 runs. It takes 0.5 s on main and here. The three collected-Responsecases that main already has pass on that build, so this is specific to a body that is complete and held when theResponseis collected. I do not know if that is an intended change in #43169 or a hang. The test helper edits (serveConnectProxycounts CONNECTs) are the same lines in both PRs.Where the numbers come from. Release builds of fd8422c (then main) with and without this diff, and of b52d513 (before #43123). This PR is the same diff on 367d939: it applied without changes, and none of the commits in between touch these hunks. I did not measure again after that rebase. Client CPU is
process.cpuUsage()per request over keep-alive fetches of one gzip body. The origin is a separate process on other cores. The host is shared and ran at load 40 to 75. A 7-run median moved by about 13% between runs there, so only ratios inside one run are comparable.Median us CPU per request, 9 interleaved runs x 1000 requests,
arrayBuffer(), before #43123 / main / this diff:Main is 1.22 to 1.36x here. This diff is within noise of the cost before #43123 on every size in the band. The ratios below 1.00 are noise, not a speedup: this diff does two more thread hops per response than the old code.
text()gives the same shape (1 MiB 569 / 704 / 570). So does an origin that sends the head a tick before the body (1 MiB 323 / 442 / 316), which is the path where the hold happens with no callback.A
res.bodyreader costs the same as on main (1 MiB 629 / 585, 4 MiB 1,969 / 1,972). A 300 B keep-alive body, 15 runs x 3000 requests in two orders: 43 / 44 / 44 and 45 / 46 / 45.I did not use
bench/snippets/fetch-gzip.mjs. It runs the server in the client process and reports wall time. On this host its unchanged rows moved by 20 to 27% between binaries, so it could not resolve the effect.What the tests prove. One case discriminates, and only in a debug build: it reads the
HTTPInternalStatedebug log and expects oneDecompressing N bytes with libdeflateline. On main it also seesDecompressing 6167 bytesandDecompressing 4583 bytes, the two zlib passes. From JavaScript the two paths deliver the same bytes by design, so nothing else can tell them apart. The other 22 cases pass on main too. They guard the new state: both framings, a buffered consumer and a reader, the body arriving with the head, after the head, and after the consumer, TLS, a CONNECT tunnel, an origin that closes the connection or the tunnel before a consumer attaches, aResponsenobody reads (the process exits), and a collectedResponse(the fetch is aborted).The CONNECT proxy cases clear
NO_PROXYand the proxy variables for the child and assert that the proxy saw one CONNECT. An ambientNO_PROXYthat lists 127.0.0.1 makesfetch()ignore itsproxyoption, and the case then passes without a tunnel. I observed 0 CONNECTs that way and 1 with the variables cleared.Please check this one.
handle_response_body_from_multiple_packetsno longer setsis_libdeflate_fast_path_disabledafter a pass over the final chunk. It sets it before a pass over a non-final chunk only. A held body needs the flag to stay clear so that the later pass can use libdeflate. I believe the rest is unchanged:decompress_bytessets the flag itself when it enters the libdeflate block, and every later call sees a decoder that is notNone. The chunked paths never set it after a final chunk.At the end of the transport. The first version of this description said that nothing is decoded for a
Responsethat nobody reads. That is true only while the connection is open. A review pointed out thatfinalize_body_on_eofdecodes whatever is held with no budget, and a held body reaches it whole. Through a CONNECT tunnel the socket is never paused, so an origin that closes an idle connection gets there before any consumer. I confirmed it: ares.bodyreader whose tunnel closed first got its body from oneDecompressing 6167 bytes with libdeflatecall. Main has the same gap for the part of a body it has not decoded yet (#43123 lists it as still unbounded), and the hold never decodes more than main does. It does not close the gap. #43169 is the change that bounds it, so I did not copy that work here. The four close cases assert exact bytes, not memory.What the hold costs. A reader of a held body gets its first chunk one thread round trip later. A held body that nobody reads keeps its connection with nothing decoded, where main keeps it with 256 KB decoded. For a gzip stream with a truthful trailer, the set of bodies is a subset of what #43123 already parks (a decoded size of 256 KB or more). The hold trusts the trailer. A stream whose trailer overstates its size is held too, where main would decode it and free the connection. That gives a server nothing new: it can already make a client park a connection with a real body of that size, which is a few KB on the wire.
Unchanged. Bodies up to 512 KB, deflate, brotli and zstd, HTTP/2 and HTTP/3 (they start
Unclaimedbut are never held, because their output cap is unbounded), S3 (itsStorestill startsFlowing), and any body that arrives in more than one read.