perf(transport): take HTTP/2 response headers in wire case - #113
Open
burruplambert wants to merge 3 commits into
Open
perf(transport): take HTTP/2 response headers in wire case#113burruplambert wants to merge 3 commits into
burruplambert wants to merge 3 commits into
Conversation
The fork's read loop canonicalised every response field name, one allocated string per field, and buildHeadersMap immediately lowercased each one back for the map this library hands its callers. The transport now asks the fork for the map keyed as the wire carried the names (WireCaseResponseHeaders), so they flow through untouched. Content-Encoding is the one header the transport reads from the raw map before conversion, and http.Header.Get canonicalises its argument, so it would miss the wire-cased keys and hand back a compressed body with no error anywhere. It resolves through responseContentEncoding, which addresses both key sets because HTTP/1.1 parses into canonical case and HTTP/3 still canonicalises. A wire-level test locks headers, order and the gzip unwrap. BenchmarkBuildHeadersMapCanonical 33 allocs/op 1616 B/op BenchmarkBuildHeadersMapWireCase 19 allocs/op 1368 B/op
The wire-case change audited the transport's readers of the raw response map but not the pooling client's, and that client reads more than Content-Encoding: redirect Location, Set-Cookie capture and the digest challenge all indexed by canonical name and went quiet on HTTP/2, which the client package's own redirect tests catch. Every raw-map reader in the module now resolves through internal/respheader, which holds both spellings of each name as constants so neither lookup converts or allocates. Its test derives the canonical form with textproto rather than trusting the literal, which is what caught WWW-Authenticate canonicalising to Www-Authenticate. The local proxy canonicalises names as it relays a response onto its HTTP/1.1 client, so the bytes it writes stay what they always were.
|
@burruplambert is attempting to deploy a commit to the sardanioss' projects Team on Vercel. A member of the Team first needs to authorize it. |
With WireCaseResponseHeaders on, the fork keys the response map by the wire's field names, and its framer rejects a response field name that is not lowercase (RFC 9113 8.2.1), so the map already has exactly the shape buildHeadersMap was copying it into. The copy had nothing left to convert; it existed so the transport's bookkeeping entries would not show through to callers. The HTTP/2 paths, buffered and streaming, now take the fork's map itself and delete the bookkeeping entries. That is sound on three verified grounds. The fork keeps no reader or writer on the map once the response is handed over: trailers accumulate in a map of their own, and the content-length and content-encoding looks happen while the response is still being built. The keys are guaranteed lowercase by the framer. And the fork pins every entry's capacity inside its shared value backing, so appending through one entry cannot reach another, the same isolation a fresh copy per entry gave. The wire order and HTTP/1.1 casing are read out before the entries carrying them are deleted. HTTP/1.1 still arrives canonical-cased through textproto and HTTP/3 canonicalised by quic-go, so both keep the lowercase copy. Conversion per fourteen-header response, order copy included: 4782ns and 20 allocations to 981ns and 1; the map copy itself 19 allocations to none. The wire-level test locks the caller-visible contract and a unit differential pins adoption to what the copy produced.
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.
Depends on sardanioss/net#2, which adds the WireCaseResponseHeaders field this change sets; go.mod needs the next net tag before this builds, same pairing as the 1.7.0 fork bumps.
The fork's HTTP/2 read loop canonicalised every response field name, one allocated string per field, and buildHeadersMap immediately lowercased each one back for the map this library hands its callers. With the flag set the names flow through untouched: the fork allocates nothing per name and buildHeadersMap's lowercasing hands the same string back.
The delicate part is every reader of the raw response map between the transport and the conversion, because http.Header.Get canonicalises its argument and cannot find wire cased keys. All of them were audited and now resolve through internal/respheader, which holds both spellings of each name as constants so neither lookup converts or allocates: Content-Encoding in the transport and both clients, and the pooling client's redirect Location, Set-Cookie capture and digest Www-Authenticate. That last name is why respheader's test derives each canonical form with textproto rather than trusting a literal: WWW-Authenticate canonicalises to Www-Authenticate, and a literal lookup never matched a parsed map. The local proxy canonicalises names as it relays a response onto its HTTP/1.1 client, so the bytes it writes stay what they always were. HTTP/1.1 and HTTP/3 still produce canonical keys and the same accessors serve them, and the client package's redirect tests fail if any of this regresses.
Nothing observable changes for callers: response Headers were lowercase before and after, HeaderOrder is unchanged, and decompression still triggers, locked by a wire level test against an in-process HTTP/2 server.
BenchmarkBuildHeadersMapCanonical 33 allocs/op 1616 B/op
BenchmarkBuildHeadersMapWireCase 19 allocs/op 1368 B/op
with the per field canonicalisation the fork no longer performs on top of that.
A third commit finishes the thought: with the map already keyed by the wire's lowercase names, buildHeadersMap had nothing left to convert, yet still built a second map and copied every value slice so the transport's bookkeeping entries would not show through. The HTTP/2 paths, buffered and streaming, now take the fork's map itself and delete the bookkeeping entries. That is sound on three verified grounds: the framer rejects a response field name that is not lowercase, so the keys already have the exact shape the copy produced; the fork keeps no reader or writer on the map once the response is handed over, trailers accumulate in a map of their own and the content-length and content-encoding looks happen while the response is still being built; and the fork pins every entry's capacity inside its shared value backing, so appending through one entry cannot reach another, the same isolation a copy gave. The wire order and HTTP/1.1 casing are read out before the entries carrying them are deleted, and HTTP/1.1 and HTTP/3 keep the lowercase copy. Wire level tests over an in-process HTTP/2 server lock the caller contract on both the buffered and streaming paths, and a unit differential pins adoption to what the copy produced.
BenchmarkResponseHeaderConversion/copy 4782 ns/op 20 allocs/op
BenchmarkResponseHeaderConversion/adopt 981 ns/op 1 allocs/op
(order copy included in both; the map copy itself goes from 19 allocations to none)