http: replace picohttpparser with a Rust parser; classify headers once and build fetch Response headers lazily - #37132
Conversation
…rser and chunked decoder
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
…way kernels No-Verification-Needed: CI allowlist data only, no product code
…tchHeaders only when a Response's headers are actually read
…new bytes could complete it
dylan-conway
changed the title
WalkthroughThe change replaces picohttpparser with Rust HTTP response parsing and chunked decoding. It adds typed header classification, Highway control-byte scanning, deferred response-header materialization, updated WebCore bindings, build cleanup, benchmarks, and expanded fetch tests. ChangesHTTP parser and decoder
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/picohttp/lib.rs (1)
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the stale
.bssclaim onZERO.The comment states that
ZERO"evaluates to all-zero bytes" so[Header::ZERO; N]statics land in.bss.name_idis nowSelf::OTHER(u8::MAX), so the const is no longer all-zero and such arrays move to.data.OTHERcannot be0, because0is the discriminant ofHeaderName::Accept. Update the comment so the null-pointer rationale is not attributed to a section placement that no longer applies.📝 Proposed comment fix
/// All-zero sentinel — name/value are empty slices. Used by callers to /// initialize fixed-size header arrays before filling them. /// - /// Uses `null()` (not `b"".as_ptr()`) so the const evaluates to all-zero - /// bytes — `[Header::ZERO; N]` statics land in `.bss` instead of `.data`. - /// `name()`/`value()` go through `ffi::slice`, which tolerates `(null, 0)`. + /// Uses `null()` (not `b"".as_ptr()`) so the pointer fields need no + /// relocation. `name()`/`value()` go through `ffi::slice`, which tolerates + /// `(null, 0)`. `name_id` is `OTHER`, not `0`, because `0` is + /// `HeaderName::Accept`. pub const ZERO: Self = Self {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picohttp/lib.rs` around lines 62 - 77, Update the documentation for Header::ZERO to remove the stale claim that it produces all-zero bytes or places [Header::ZERO; N] statics in .bss. Retain the explanation that null pointers represent empty slices and are supported by ffi::slice, and leave Header::OTHER unchanged because it must remain distinct from the Accept discriminant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/fetch/response-headers-server.mjs`:
- Around line 38-39: Update the request handling around the response profile
selection to parse req.url with URL and derive the profile from pathname,
excluding query parameters. Validate the resulting kind against responses and
return a clear error Response for unknown profiles instead of silently using an
unprofiled response; preserve the existing nginx default when no profile is
specified.
In `@bench/fetch/response-headers.mjs`:
- Around line 7-15: Validate the parsed CLI arguments before any benchmark
workers start: require a valid port, restrict kind to supported profiles and
mode to the supported values, and require total and conc to be positive
integers. Reject missing, non-numeric, zero, negative, fractional, or unknown
values with a clear usage error, preventing invalid profiles from reaching one()
and causing header access failures.
In `@src/http_types/HeaderName.rs`:
- Around line 116-123: Add a compile-time const assertion near
HeaderName::from_index that verifies NAMES.len() equals the number of contiguous
HeaderName variants, using the last variant’s discriminant as the expected
bound. Keep from_index’s existing COUNT check and transmute unchanged, while
ensuring mismatched names and variants fail compilation.
In `@src/http/lib.rs`:
- Around line 3681-3692: Update the response-reading logic around already_seen
and the to_read.len() < 16 guard so the may_be_complete fast path is used only
when the stored prefix was actually parsed and returned ShortRead. Do not treat
the accumulated 1–15 byte prefix from short_read! as validated; reset or
otherwise gate already_seen until parsing has occurred, while preserving normal
validation for parsed prefixes.
- Around line 4643-4685: Add the crate::Error variant for ChunkedEncodingError
and implement the required error/name mappings and traits so it can be
propagated directly. Update both chunked decoder call sites, including
handleResponseBodyChunkedEncodingFromMultiplePackets and the corresponding path
near the second decoder invocation, to return ChunkedEncodingError instead of
InvalidHTTPResponse when decoding fails.
In `@src/runtime/webcore/Response.rs`:
- Around line 1464-1465: Update Init::clone to take &mut self, call
self.realize_headers() before cloning, and retain the existing cloning behavior
afterward so deferred wire_headers are materialized into headers instead of
being dropped.
In `@test/js/web/fetch/chunked-trailing.test.js`:
- Line 681: Update the raw TCP socket error handlers in
test/js/web/fetch/chunked-trailing.test.js:681 and
test/js/web/fetch/client-fetch.test.ts:710 to tolerate only expected
client-abort errors such as ECONNRESET or EPIPE. Propagate every other socket
error through the awaited helper or test while preserving the original error
object.
---
Outside diff comments:
In `@src/picohttp/lib.rs`:
- Around line 62-77: Update the documentation for Header::ZERO to remove the
stale claim that it produces all-zero bytes or places [Header::ZERO; N] statics
in .bss. Retain the explanation that null pointers represent empty slices and
are supported by ffi::slice, and leave Header::OTHER unchanged because it must
remain distinct from the Accept discriminant.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b498bfc0-e973-4e16-b323-c9c63140ea52
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/miri.yml.gitignoreCLAUDE.mdLICENSE.mdbench/fetch/response-headers-server.mjsbench/fetch/response-headers.mjsdocs/project/license.mdxscripts/build/bun.tsscripts/build/deps/index.tsscripts/build/deps/picohttpparser.tsscripts/build/flags.tsscripts/build/source.tsscripts/rust-miri.tsscripts/verify-baseline-static/allowlist-aarch64.txtscripts/verify-baseline-static/allowlist-x64-windows.txtscripts/verify-baseline-static/allowlist-x64.txtsrc/highway/lib.rssrc/http/InternalState.rssrc/http/lib.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/http_types/HeaderName.rssrc/http_types/Method.rssrc/http_types/lib.rssrc/jsc/FetchHeaders.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/bindings.cppsrc/jsc/bindings/highway_strings.cppsrc/picohttp/Cargo.tomlsrc/picohttp/chunked.rssrc/picohttp/lib.rssrc/picohttp/parse.rssrc/runtime/api/html_rewriter.rssrc/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/wyhash/lib.rstest/js/node/process/process.test.jstest/js/web/fetch/chunked-trailing.test.jstest/js/web/fetch/client-fetch.test.tstest/js/web/fetch/fetch.test.ts
💤 Files with no reviewable changes (6)
- .gitignore
- src/http_types/Method.rs
- src/jsc/bindings/BunProcess.cpp
- scripts/build/deps/picohttpparser.ts
- test/js/node/process/process.test.js
- scripts/build/flags.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…gate the head prefilter on a parsed prefix, realize headers inside Init::clone
No-Verification-Needed: benchmark script only
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/http/lib.rs (2)
3668-3675: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce the response-header limit before staging input.
When this read still leaves the head incomplete, Line 3634 appends the full
incoming_databefore the 1 MiB check in Lines 3670-3672. The staging allocation can exceedMAX_RESPONSE_HEADER_BUFFERby the full packet size. Bound the bytes copied into the staging buffer beforeappend_slice_exact. Preserve packets that contain the completed head and body by staging only the bounded head prefix and processing the remaining body bytes separately.As per coding guidelines, validate untrusted input before allocation and enforce actual resource limits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/lib.rs` around lines 3668 - 3675, Update the incomplete response-head handling around the `incomplete_head!` macro and the `append_slice_exact` call so the 1 MiB `MAX_RESPONSE_HEADER_BUFFER` limit is checked before copying `incoming_data` into the staging buffer. Stage only the bounded head prefix, and process any remaining bytes as body data when the packet completes the head; preserve existing behavior for packets containing both the completed head and body.Source: Coding guidelines
4790-4796: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winParse
Content-Typebefore enabling SSE mode.
starts_with_case_insensitive_asciimatchestext/event-streamingand other invalid media types. If this setsis_server_sent_events, a zero-length response can wait for connection close instead of completing. Parse the media type and enable SSE only for the exacttext/event-streamtype with valid parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/lib.rs` around lines 4790 - 4796, Update the ContentType branch in the header-processing logic to parse the media type before setting is_server_sent_events. Enable SSE only when the parsed type is exactly text/event-stream and its parameters are valid, rejecting values such as text/event-streaming while preserving normal response completion behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/fetch/response-headers.mjs`:
- Line 11: Update the CLI validation condition in the response-header benchmark
to require port to be an integer from 1 through 65535 using Number.isInteger,
and require both N and C to be safe integers at least 1 using
Number.isSafeInteger. Preserve the existing mode validation and rejection
behavior.
In `@src/jsc/bindings/bindings.cpp`:
- Around line 2149-2155: Update the header conversion logic around
HTTPHeaderName::SetCookie so every Set-Cookie value is appended to the dedicated
Set-Cookie collection rather than passed to HTTPHeaderMap::add, preserving
repeated fields separately. When constructing FetchHeaders, transfer both the
HTTPHeaderMap and the populated Set-Cookie collection so getSetCookie() returns
all values. Add a regression test covering multiple Set-Cookie fields.
---
Outside diff comments:
In `@src/http/lib.rs`:
- Around line 3668-3675: Update the incomplete response-head handling around the
`incomplete_head!` macro and the `append_slice_exact` call so the 1 MiB
`MAX_RESPONSE_HEADER_BUFFER` limit is checked before copying `incoming_data`
into the staging buffer. Stage only the bounded head prefix, and process any
remaining bytes as body data when the packet completes the head; preserve
existing behavior for packets containing both the completed head and body.
- Around line 4790-4796: Update the ContentType branch in the header-processing
logic to parse the media type before setting is_server_sent_events. Enable SSE
only when the parsed type is exactly text/event-stream and its parameters are
valid, rejecting values such as text/event-streaming while preserving normal
response completion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36e1b9c2-b167-415b-8278-bef561fc0a78
📒 Files selected for processing (7)
bench/fetch/response-headers-server.mjsbench/fetch/response-headers.mjssrc/http/lib.rssrc/http_types/HeaderName.rssrc/jsc/bindings/bindings.cppsrc/picohttp/lib.rssrc/runtime/webcore/Response.rs
…actly text/event-stream
|
On the two outside-diff notes in the last review: the SSE check now requires the media type to be exactly |
No-Verification-Needed: benchmark script only
There was a problem hiding this comment.
No bugs found, but this is a full HTTP/1.1 response-head parser and chunked-decoder rewrite plus lazy FetchHeaders materialization on every fetched Response — hot-path, protocol-boundary code that warrants a human sign-off.
What was reviewed
- The new parser/decoder against RFC 9112 framing rules and the ported picohttpparser test cases; SWAR
index_of_line_endand the highway CTL kernel match the scalar predicate. HeaderName::classify/from_index: the transmute is guarded by theCOUNT == XXSSProtection+1const assert and the round-trip test; thename_id = enum+1encoding lines up with C++header.name_id - 1andHeader::ZEROis all-zero again.- Lazy
wire_headers: everyinit.headersaccessor routes throughrealized_init()/realize_headers(),Init::clonerealizes first,set_init_headersclearswire_headers, and the repeated-Content-Typepath falls back to the map so combining is consistent. may_be_completeprefilter: theseen >= 16gate now excludes prefixes that never reachedparse;already_seenis captured before the 16-byte floor short-reads.
Extended reasoning...
Overview
This PR replaces the vendored C picohttpparser with a Rust HTTP/1.1 response-head parser (src/picohttp/parse.rs) and chunked-body decoder (src/picohttp/chunked.rs), adds a length-bucketed HeaderName::classify so field names are tagged once at parse time, wires that tag through to createFromPicoHeaders_ in bindings.cpp (skipping findHTTPHeaderName and the map's duplicate scan for first occurrences), and defers building FetchHeaders for a fetched Response until something reads them (Init::wire_headers → realize_headers()). It also adds a highway IndexOfHttpCtl kernel with an inline two-word SWAR fast path, a may_be_complete prefilter so a trickled head isn't re-parsed quadratically, tightens the SSE Content-Type check to an exact media-type match, and removes the picohttpparser dep (build plumbing, process.versions.picohttpparser, license entry, allowlists). 42 files across src/picohttp, src/http, src/http_types, src/highway, src/runtime/webcore, src/jsc/bindings, build scripts, and tests.
Security risks
The parser sits directly on untrusted network bytes. I checked: bounds are enforced via Cursor::peek/buf.get() (no raw pointer arithmetic on the wire buffer); chunk-size is capped at MAX_HEX_DIGITS before it can overflow usize; obs-fold and bare-LF chunk framing are now rejected (stricter than before, matching Node — reduces smuggling surface); HeaderName::from_index transmutes only after a < COUNT check backed by a const assert, and well_known() uses wrapping_sub(1) so name_id == 0 maps to 255 → None. The new C++ path casts name_id - 1 to HTTPHeaderName under an ASSERT(index < numHTTPHeaderNames) — release builds rely on the Rust side never producing an out-of-range id, which classify cannot. No new unbounded allocation paths; the 1 MiB head cap is unchanged.
Level of scrutiny
High. This is the response parser for fetch, bun install's registry client, and the WebSocket upgrade handshake — every byte off the wire flows through it, and the lazy-headers change threads a new invariant ("every init.headers read goes through realize_headers()") through Response, Body, FetchTasklet, RequestContext, and html_rewriter. The PR is thoroughly tested (Miri, differential fuzzing against the C library, per-byte and every-split-point unit tests, JS integration tests for each deferred-headers path), and all prior review threads are resolved, but a parser rewrite of this scope is exactly the kind of change a maintainer should sign off on rather than an automated approval.
Other factors
- All eight prior review threads (my
Header::ZEROcomment and CodeRabbit's findings onmay_be_complete,Init::clone, Set-Cookie, bench-script validation) are addressed and resolved; the two intentionally left as-is have stated rationale. - CI is 195/196 green with the remaining failure attributed to a known unrelated flake.
- User-visible behavior changes:
process.versions.picohttpparseris removed, chunked bodies with bare-LF framing or non-hex garbage after chunk-size are now rejected, andtext/event-stream-foono longer triggers SSE handling. All are defensible tightenings but worth a human ack.
1 participant
What does this PR do?
Bun's HTTP client (fetch,
bun install, the WebSocket upgrade client) parsed HTTP/1.1 response heads andTransfer-Encoding: chunkedbodies with the vendored C library picohttpparser. This replaces both entry points we used with Rust inbun_picohttp, drops the vendored dependency (build plumbing,process.versions.picohttpparser), and then uses owning the parser to cut the per-response header work in fetch.Commit 1 — parser/decoder replacement (behavior parity)
Response::parsevalidates every byte it has, rejects obs-fold inline, and scans field values with a new highway kernel (highway_index_of_http_ctl) plus a two-word inline fast path, so arm64 gets SIMD too (pico only had SSE4.2).ChunkedDecoderfollows upstream picohttpparser HEAD rather than our 2021 snapshot: CRLF required after chunk-size and chunk-data (bare LF /CR CR LFrejected, as Node does), non-hex garbage after chunk-size rejected. Upstream's framing-overhead heuristic is deliberately not carried over (it rejects long streams of 1-byte chunks).provides.sourcesdep mechanism is removed fromscripts/build.Commit 3 — classify once, materialise lazily
HTTPHeaderNameset once, inHeader::new(bun_http_types::HeaderName::classify, length bucket → one byte → one case-insensitive compare).handle_response_metadata/build_requestswitch on the tag instead of a cascade of wyhash compares, andcreateFromPicoHeadersskipsfindHTTPHeaderNameand the map's duplicate scan for first occurrences.Responsekeeps the parsed head (Init.wire_headers) and only buildsFetchHeaderswhen something reads them;blob()/formData()getContent-Typefrom the wire form directly. Every accessor that hands outinit.headersgoes throughrealize_headers().Commit 4 — when a head arrives across many reads, only re-parse once the new bytes could have completed it (port of pico's
is_complete(last_len), which Bun never wired up), so a trickling server can't make header parsing quadratic.What gets faster, in user terms
This is all on the client side of
fetch()(the work Bun does per response, split between its HTTP thread and the JS thread). Nothing changes forBun.serve, and the network round-trip itself is untouched, so a singleawait fetch()against a remote server won't feel different. Where it shows up is code that does a lot of fetches — crawlers, proxies, API fan-out,bun install-style workloads — because each response now costs less CPU, so the same core sustains more requests/s.Measured with release builds of this branch vs. its merge-base against a local server sending a typical API/CDN-style response (14 headers:
Content-Type,Cache-Control,ETag,Strict-Transport-Security, a fewX-*, …) with a tiny body, 64 requests in flight:const r = await fetch(u); await r.arrayBuffer()— headers never touchedr.headers.get("content-type")for (const [k, v] of r.headers)(The rest of the per-response CPU is syscalls and the fetch/promise machinery, which this PR doesn't touch.) In that benchmark the client was already able to push ~5–6% more requests/s through a server that was the bottleneck. Responses with only 2–4 headers have less to save: −3 to −5% CPU when headers aren't read, no measurable change when they are.
res.blob()/res.formData()count as "headers never touched" — they readContent-Typewithout building theHeadersobject.Two pathological cases also improve: a response head that arrives in many small reads (slow or adversarial server) no longer gets re-parsed from the start on every read — a 900 KB head in 512-byte writes went from 147 ms to 62 ms of client CPU — and long streams of tiny chunked-encoding chunks are decoded byte-for-byte as before rather than being rejected by upstream pico's new overhead heuristic.
The parser itself, measured standalone against the C library it replaces, is at parity (a bit faster on short heads and long values, ~1.15× slower on many-short-header heads, chunked decoding within ~10%); the wins above come from doing less with the headers after parsing, not from parsing faster.
How did you verify your code works?
test.cplus new ones (parser, decoder, classifier,may_be_complete), run under Miri (bun run rust:miri; crate added to the Miri set).bun bd teston fetch / body / response / blob / chunked-trailing / client-fetch / fetch.stream / fetch-redirect / fetch-gzip / fetch-http2-client / websocket-client / node-http / serve / bun-serve-file / html-rewriter / wasm-streaming / bun-install-registry; new JS tests for strict chunk framing, CTL handling in long values, the deferred-headers paths (blob type, clone,new Response(x, res), served back out ofBun.serve, HTMLRewriter, repeatedContent-Type), and a byte-per-write head.bun run rust:check-all(all targets), clang-format/rustfmt/prettier clean.fetch.test.tshas 2–3 TLS tests and one s3 multipart test that time out / flake identically on an unmodified main debug build.Not done here (possible follow-ups): letting
FetchHeaders.get()answer from the wire block without building the map, and a single-allocationclone_metadata.worker-transfer-terminate-stress.test.tson the x64-asan lane, which its own header describes as an intermittent MessagePort/terminate abort and which fails on unrelated branches this week (builds 90005, 89980, 89893).