http: route identity chunked bodies through the multi-packet decode path - #35411
http: route identity chunked bodies through the multi-packet decode path#35411robobun wants to merge 5 commits into
Conversation
handle_response_body_chunked_encoding_from_single_packet decodes into a 16 KiB scratch buffer, then either hands the decoded bytes straight to decompress_bytes (compressed: skips allocating compressed_body) or append_slice_exact's them into body_out_str (identity: scratch copy followed by a second memcpy into the destination). _from_multiple_packets appends the raw bytes into body_out_str once, decodes in the tail, and truncates. For identity bodies that is one memcpy instead of two, so gate the single-packet dispatch on encoding.is_compressed(). bench/snippets/fetch-chunked-small.mjs covers 256 B / 4 KiB / 15 KiB chunked bodies under identity and gzip. On a release build the change is inside run-to-run noise: the avoided memcpy is <=1.5 us against a ~40 us localhost round-trip.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 5:02 AM PT - Jul 24th, 2026
❌ @autofix-ci[bot], your commit 087ed6b has 1 failures in 🧪 To try this PR locally: bunx bun-pr 35411That installs a local version of the PR into your bun-35411 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the dispatch change looks correct and my earlier nit about the test comment was addressed in 2181ace. Deferring to a maintainer only because the PR explicitly leaves the merge/close call open ("happy to close if that's preferred") given the bench shows the memcpy win is below the noise floor; that's a judgment call I shouldn't make.
What was reviewed:
- Traced identity bodies through
_from_multiple_packets→process_body_buffer: for identity it just moves the takenbody_out_str.listback, so net is one append + decode-in-tail as described. - Checked the redirect-pending and
body_out_str == Nonebranches inprocess_body_buffer— both already handled identity bodies on this path for >16 KiB / second-packet cases, so no new behavior. - Confirmed the gzip path is untouched (
is_compressed()still routes to_from_single_packet).
Extended reasoning...
Overview
The functional change is a single added conjunct in handle_response_body_chunked_encoding (src/http/lib.rs:4788-4794): the _from_single_packet fast path is now taken only when encoding.is_compressed(), so small identity-encoded chunked bodies route through _from_multiple_packets instead. A new bench snippet and 8 new regression tests (identity/gzip × 4 sizes) accompany it. This is a follow-up to a review note on #35373.
Security risks
None. This is an internal dispatch decision between two existing decode paths; no new parsing, no new inputs, no boundary changes.
Level of scrutiny
Medium — it touches the fetch HTTP client's chunked-body hot path, but the change is strictly narrowing which inputs take an existing shortcut. The _from_multiple_packets path already handled identity bodies for the >16 KiB case and for every packet after the first (when get_body_buffer().list is non-empty), so small first-packet identity bodies are being routed onto a well-exercised path. I traced the identity Done arm through process_body_buffer (src/http/InternalState.rs:447-453) and confirmed it's a move-back no-op, matching the PR's one-memcpy claim. The redirect-pending and None-body_out_str guards in process_body_buffer cover the same edge cases the single-packet path's is_redirect_pending check covered.
Other factors
The bench honestly reports the win is below run-to-run noise (~1.5 µs saved vs ~40 µs variance), and the author explicitly says "happy to close if that's preferred." That's a request for maintainer judgment on whether the code-clarity benefit (making the scratch path's compressed-only purpose explicit) is worth the churn — not something a bot should decide. My prior inline nit about the test comment overclaiming -2 coverage was addressed by trimming the comment in 2181ace; the thread is resolved. Tests pass per the PR body.
fetch-retry-chunked.test.ts is scoped to the BUN-3BZF retry guard; fetch-gzip.test.ts already hosts the chunked + Content-Encoding decode coverage and has createNetServer / the chunked framing pattern. Collapsed the four gzip sizes into one control case using incompressible randomBytes so the compressed payload is actually multi-KiB rather than a ~50 B gzipped run of 'x'.
|
Addressed the review feedback:
This is a behaviour-preserving dispatch change (the new tests pass on CI on 087ed6b: |
There was a problem hiding this comment.
No issues found. Deferring the merge/close call to a maintainer since this is the bench follow-up requested in #35373 and the results are within noise — that judgment call isn't mine to make.
What was reviewed:
- The added
is_compressed()gate:_from_multiple_packetsis already the identity-chunked path for >16 KiB bodies, so routing small ones there is not new territory; confirmedchunked_decoder_and_body_buffer()returnsbody_out_strfor identity andprocess_body_buffer's identity arm moves the taken Vec straight back. - New tests:
serve()destroys tracked sockets beforeserver.close()so the keep-alive connection doesn't leak; the earlier -2 comment nit was addressed in 2181ace. - The
randomBytes-derived gzip test name varies per run — flagged by finders but ruled out as harmless (no snapshot, not a-tfilter target).
Extended reasoning...
Overview
Three files: a one-condition change to the chunked-body dispatch in src/http/lib.rs (add self.state.encoding.is_compressed() && to the _from_single_packet gate), a new bench snippet under bench/snippets/, and four regression tests appended to test/js/web/fetch/fetch-gzip.test.ts. The change is behaviour-preserving by construction — _from_multiple_packets is already the handler for identity chunked bodies whenever the first read exceeds 16 KiB or the body buffer is non-empty; this just sends the ≤16 KiB first-read identity case there too.
Security risks
None. No user-controlled input reaches new parsing code; the same phr_decode_chunked runs over the same bytes, just from a different call site that already existed. No auth, crypto, or bounds arithmetic is touched.
Level of scrutiny
Medium. The dispatch itself is a hot-path conditional in the HTTP client, but the change only narrows which of two existing, already-tested branches runs. I traced the identity path through chunked_decoder_and_body_buffer() → append → in-place decode → process_body_buffer and it's the same sequence already used for larger identity bodies. The bench and tests are additive.
Other factors
The author explicitly wrote "Leaving the merge/close call to a maintainer" and "happy to close if that's preferred" — the PR exists to deliver the release-build bench a maintainer asked for in #35373, and the numbers show the memcpy saving is below the loopback fetch() noise floor. Whether to land it for the code-clarity benefit (making the scratch path's compressed-only purpose explicit) or close it as not-worth-the-churn is a maintainer judgment call, not a correctness question. Approving would short-circuit that. My earlier inline nit (stale -2 claim in the test comment) was addressed and the thread is resolved.
2 participants
Follow-up to the review note in #35373 (comment).
What changes
handle_response_body_chunked_encodingnow takes the_from_single_packetbranch only whenencoding.is_compressed(). Identity-encoded chunked bodies of any size go through_from_multiple_packets.Why
_from_single_packetdecodes the raw chunked bytes into a 16 KiB scratch buffer and then, on the Done path, callshandle_response_body_from_single_packet:decompress_bytes(scratch, body_out, true). The compressed bytes never touchcompressed_body, so that allocation is skipped entirely. This is the reason the scratch path exists.body_out.append_slice_exact(scratch). That is a second memcpy on top of the scratch copy._from_multiple_packetsdoesbody_out.append_slice(incoming)+phr_decode_chunkedin the tail +truncate. For identity bodies that is one memcpy into the final destination and no scratch.The
-2(needs more data) case is the same shape: single-packet does scratch copy +append_slice_exact; multi-packet doesappend_slice+ decode-in-tail. One memcpy fewer.Redirect handling is unchanged:
_from_multiple_packetsalready runs for identity chunked bodies above 16 KiB, andprogress_update/state.reset()discard any bytes decoded intobody_out_strbefore the redirect fires.Bench
bench/snippets/fetch-chunked-small.mjs(new) serves 256 B / 4 KiB / 15 KiB chunked bodies over a keep-alivenetsocket under both identity and gzip encodings, headers and body written in onesock.write(). Path-dispatch counters added during verification confirmed the identity cases enter_from_multiple_packetsand the gzip cases still enter_from_single_packet.Release build, 8 alternating A/B runs, min-of-8 avg-per-iter:
Run-to-run spread on the same binary is ~40 µs (CPU frequency swings on a shared host), so all of the above is noise; the medians go both directions by similar amounts. The avoided memcpy is at most 16 KiB (~1.5 µs) against a ~38-48 µs localhost round-trip, which is below the variance floor of a loopback
fetch()bench. No regression in any measured case.So: the operation-count win is real (one memcpy instead of two, no scratch touch) but not observable at the
fetch()level. Leaving the dispatch gated onis_compressed()regardless because it makes the single-packet path's purpose explicit and costs nothing; happy to close if that's preferred.Tests
test/js/web/fetch/fetch-gzip.test.tsgains asmall chunked body decodeblock: identity at 1 B / 4 KiB / 15 KiB plus one gzip control (randomBytes so the compressed payload is actually ~4 KiB rather than a ~50 B gzipped run of the same byte). These are regression guards for the newly-routed identity path; they pass onmainand here by construction since the change is behaviour-preserving.