From 00f8cfa3d3d52fe5181fd58181d47bdebf8b080e Mon Sep 17 00:00:00 2001 From: Burrup Lambert Date: Tue, 28 Jul 2026 15:10:20 +0100 Subject: [PATCH 1/2] fix: reset stale order on pooled headerSorter in SortedKeyValues headerSorter instances are pooled and shared between SortedKeyValuesBy and SortedKeyValues, but SortedKeyValues never cleared hs.order. A sorter previously used for an ordered sort would keep its order map and apply it to a later orderless sort, producing the wrong header order whenever the new header's lowercased keys collide with entries in the stale order map. Reset hs.order before sorting in SortedKeyValues and add a regression test. --- header.go | 4 ++++ header_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/header.go b/header.go index b6481131..9561f97c 100644 --- a/header.go +++ b/header.go @@ -223,6 +223,10 @@ func (h Header) SortedKeyValues(exclude map[string]bool) (kvs []HeaderKeyValues, mutex.RUnlock() } hs.kvs = kvs + // Reset any order left on the sorter by a previous SortedKeyValuesBy + // call, otherwise a pooled sorter sorts by the stale order instead of + // lexicographically. + hs.order = nil sort.Sort(hs) return kvs, hs } diff --git a/header_test.go b/header_test.go index 3d9361bd..066950b5 100644 --- a/header_test.go +++ b/header_test.go @@ -359,3 +359,28 @@ func TestHTTP1HeaderOrder(t *testing.T) { t.Fatalf("got:\n%swant:\n%s", buf.String(), expected) } } + +func TestHeaderSorterPoolReuse(t *testing.T) { + // A sorter used by SortedKeyValuesBy keeps its order map. When it is + // pulled from the pool again by SortedKeyValues (no order), the stale + // order must not be used. + ordered := Header{ + "Zebra": {"1"}, + "Apple": {"2"}, + } + // Deliberately the reverse of lexicographic order. + _, hs := ordered.SortedKeyValuesBy(map[string]int{"zebra": 0, "apple": 1}, nil) + headerSorterPool.Put(hs) + + plain := Header{ + "Apple": {"1"}, + "Banana": {"2"}, + "Zebra": {"3"}, + } + kvs, _ := plain.SortedKeyValues(nil) + for i := 1; i < len(kvs); i++ { + if kvs[i-1].Key > kvs[i].Key { + t.Fatalf("keys not sorted lexicographically: %q before %q", kvs[i-1].Key, kvs[i].Key) + } + } +} From 985792cd2ff72bb17057d2f99f40291a9a84e456 Mon Sep 17 00:00:00 2001 From: Burrup Lambert Date: Fri, 31 Jul 2026 15:38:46 +0100 Subject: [PATCH 2/2] perf: do header clone, augment and sort once in http2 encodeHeaders encodeHeaders enumerates the request headers twice - once to count the header list size against peerMaxHeaderListSize, once to write them. The enumerate closure cloned req.Header, added content-length and accept-encoding, built the Header-Order: index map and ran the full sort on every call, so all of that work ran twice per request. Hoist the clone/augment/sort out of the closure so it runs once and both passes iterate the precomputed result. Also pass nil instead of an empty exclude map to SortedKeyValues/SortedKeyValuesBy - the map was allocated per request and only ever read. The emitted header sequence is unchanged, including the existing behavior that the magic Header-Order:/PHeader-Order: entries are counted by the size pass and skipped by the write pass. Mirrored in h2_bundle.go. Adds BenchmarkClientConnEncodeHeaders, which measures header encoding directly; the existing BenchmarkClientRequestHeaders cannot run here because a round trip without a configured pseudo-header order fails (pre-existing issue). --- h2_bundle.go | 50 ++++----- http2/transport_test.go | 224 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 247 insertions(+), 27 deletions(-) diff --git a/h2_bundle.go b/h2_bundle.go index 48796ba9..2ee43063 100644 --- a/h2_bundle.go +++ b/h2_bundle.go @@ -8643,6 +8643,32 @@ func (cc *http2ClientConn) encodeHeaders(req *Request, addGzipHeader bool, trail } } + // Clone, augment and sort the headers once up front: enumerateHeaders + // below is called twice (once to count the header list size, once to + // write), and this work is identical between the two passes. Cloning + // also keeps the added content-length and accept-encoding headers off + // the caller's req.Header. + hdrs := req.Header.Clone() + if _, ok := req.Header["content-length"]; !ok && http2shouldSendReqContentLength(req.Method, contentLength) { + hdrs["content-length"] = []string{strconv.FormatInt(contentLength, 10)} + } + + // Does not include accept-encoding header if its defined in req.Header + if _, ok := hdrs["accept-encoding"]; !ok && addGzipHeader { + hdrs["accept-encoding"] = []string{"gzip, deflate, br"} + } + + var kvs []HeaderKeyValues + if headerOrder, ok := hdrs[HeaderOrderKey]; ok { + order := make(map[string]int, len(headerOrder)) + for i, v := range headerOrder { + order[v] = i + } + kvs, _ = hdrs.SortedKeyValuesBy(order, nil) + } else { + kvs, _ = hdrs.SortedKeyValues(nil) + } + enumerateHeaders := func(f func(name, value string)) { // 8.1.2.3 Request Pseudo-Header Fields // The :path pseudo-header field includes the path and query parts of the @@ -8696,32 +8722,8 @@ func (cc *http2ClientConn) encodeHeaders(req *Request, addGzipHeader bool, trail f("trailer", trailers) } - // Should clone, because this function is called twice; to read and to write. - // If headers are added to the req, then headers would be added twice. - hdrs := req.Header.Clone() - if _, ok := req.Header["content-length"]; !ok && http2shouldSendReqContentLength(req.Method, contentLength) { - hdrs["content-length"] = []string{strconv.FormatInt(contentLength, 10)} - } - - // Does not include accept-encoding header if its defined in req.Header - if _, ok := hdrs["accept-encoding"]; !ok && addGzipHeader { - hdrs["accept-encoding"] = []string{"gzip, deflate, br"} - } - // Formats and writes headers with f function var didUA bool - var kvs []HeaderKeyValues - - if headerOrder, ok := hdrs[HeaderOrderKey]; ok { - order := make(map[string]int) - for i, v := range headerOrder { - order[v] = i - } - kvs, _ = hdrs.SortedKeyValuesBy(order, make(map[string]bool)) - } else { - kvs, _ = hdrs.SortedKeyValues(make(map[string]bool)) - } - for _, kv := range kvs { if strings.EqualFold(kv.Key, "host") { // Host is :authority, already sent. diff --git a/http2/transport_test.go b/http2/transport_test.go index 8490d9d0..54957d49 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -1060,7 +1060,9 @@ const ( ) // Test all 36 combinations of response frame orders: -// (3 ways of 100-continue) * (2 ways of headers) * (2 ways of data) * (3 ways of trailers):func TestTransportResponsePattern_00f0(t *testing.T) { testTransportResponsePattern(h0, h1, false, h0) } +// +// (3 ways of 100-continue) * (2 ways of headers) * (2 ways of data) * (3 ways of trailers):func TestTransportResponsePattern_00f0(t *testing.T) { testTransportResponsePattern(h0, h1, false, h0) } +// // Generated by http://play.golang.org/p/SScqYKJYXd func TestTransportResPattern_c0h1d0t0(t *testing.T) { testTransportResPattern(t, f0, f1, d0, f0) } func TestTransportResPattern_c0h1d0t1(t *testing.T) { testTransportResPattern(t, f0, f1, d0, f1) } @@ -1480,8 +1482,9 @@ func testInvalidTrailer(t *testing.T, trailers headerType, wantErr error, writeT } // headerListSize returns the HTTP2 header list size of h. -// http://httpwg.org/specs/rfc7540.html#SETTINGS_MAX_HEADER_LIST_SIZE -// http://httpwg.org/specs/rfc7540.html#MaxHeaderBlock +// +// http://httpwg.org/specs/rfc7540.html#SETTINGS_MAX_HEADER_LIST_SIZE +// http://httpwg.org/specs/rfc7540.html#MaxHeaderBlock func headerListSize(h http.Header) (size uint32) { for k, vv := range h { for _, v := range vv { @@ -4717,6 +4720,221 @@ func BenchmarkClientResponseHeaders(b *testing.B) { b.Run("1000 Headers", func(b *testing.B) { benchSimpleRoundTrip(b, 0, 1000) }) } +// BenchmarkClientConnEncodeHeaders measures request header encoding directly, +// without a round trip, in the two shapes that matter: with a Header-Order: +// key (ordered sort) and without one (lexicographic sort). +// TestClientConnEncodeHeadersOrder pins the exact header sequence +// encodeHeaders emits, across every combination of the two ordering keys +// being present or absent, so that changes to how the headers are prepared +// cannot alter what goes on the wire. +func TestClientConnEncodeHeadersOrder(t *testing.T) { + tests := []struct { + name string + pseudoOrder []string // Transport.PseudoHeaderOrder + header http.Header + method string + addGzipHeader bool + trailers string + contentLength int64 + want []string + }{ + { + // Neither ordering key, and no order on the Transport either. + // No pseudo-headers are emitted at all; that is pre-existing + // behavior on this path, unchanged by header preparation. + name: "no order keys and no transport order", + header: http.Header{"B-Two": {"2"}, "A-One": {"1"}}, + addGzipHeader: true, + contentLength: -1, + want: []string{ + "a-one: 1", "b-two: 2", + "accept-encoding: gzip, deflate, br", + "user-agent: " + defaultUserAgent, + }, + }, + { + // No ordering keys on the request, so the regular headers sort + // lexicographically and the Transport supplies the pseudo order. + name: "no order keys with transport pseudo order", + pseudoOrder: []string{":method", ":authority", ":scheme", ":path"}, + header: http.Header{"Zeta": {"z"}, "Alpha": {"a"}, "Mid": {"m1", "m2"}}, + addGzipHeader: true, + contentLength: -1, + want: []string{ + ":method: GET", ":authority: www.example.org", ":scheme: https", ":path: /p", + "alpha: a", "mid: m1", "mid: m2", "zeta: z", + "accept-encoding: gzip, deflate, br", + "user-agent: " + defaultUserAgent, + }, + }, + { + // Pseudo order only: regular headers still sort lexicographically. + name: "pheader order only", + header: http.Header{ + "Zeta": {"z"}, "Alpha": {"a"}, + http.PHeaderOrderKey: {":method", ":path", ":authority", ":scheme"}, + }, + contentLength: -1, + want: []string{ + ":method: GET", ":path: /p", ":authority: www.example.org", ":scheme: https", + "alpha: a", "zeta: z", + "user-agent: " + defaultUserAgent, + }, + }, + { + // Header order only, with keys both in and out of the list. Keys + // absent from the list follow, lexicographically. + name: "header order only, some keys unlisted", + pseudoOrder: []string{":method", ":authority", ":scheme", ":path"}, + header: http.Header{ + "In-Order": {"x"}, "Also-Order": {"y"}, + "Zeta": {"z"}, "Alpha": {"a"}, + http.HeaderOrderKey: {"in-order", "also-order"}, + }, + contentLength: -1, + want: []string{ + ":method: GET", ":authority: www.example.org", ":scheme: https", ":path: /p", + "in-order: x", "also-order: y", "alpha: a", "zeta: z", + "user-agent: " + defaultUserAgent, + }, + }, + { + name: "both order keys, with cookie splitting and content length", + header: http.Header{ + "Cookie": {"a=1; b=2;c=3"}, + "User-Agent": {"ua"}, + "Content-Type": {"application/json"}, + http.HeaderOrderKey: {"user-agent", "cookie", "content-type"}, + http.PHeaderOrderKey: {":method", ":authority", ":scheme", ":path"}, + }, + method: "POST", + contentLength: 7, + want: []string{ + ":method: POST", ":authority: www.example.org", ":scheme: https", ":path: /p", + "user-agent: ua", "cookie: a=1", "cookie: b=2", "cookie: c=3", + "content-type: application/json", + "content-length: 7", + }, + }, + { + // Connection-specific headers and Host are dropped, an empty + // value slice is skipped, and a multi-value User-Agent is cut + // to one - none of which depends on an ordering key. + name: "no order keys, filtered and skipped headers", + pseudoOrder: []string{":method", ":authority", ":scheme", ":path"}, + header: http.Header{ + "Connection": {"keep-alive"}, "Upgrade": {"h2c"}, + "Transfer-Encoding": {"chunked"}, "Host": {"other.example"}, + "Skipped": {}, + "User-Agent": {"first", "second"}, + "X-Kept": {"yes"}, + }, + contentLength: -1, + want: []string{ + ":method: GET", ":authority: www.example.org", ":scheme: https", ":path: /p", + "user-agent: first", "x-kept: yes", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse("https://www.example.org/p") + if err != nil { + t.Fatal(err) + } + method := tt.method + if method == "" { + method = "GET" + } + req := &http.Request{Method: method, URL: u, Header: tt.header} + cc := &ClientConn{ + peerMaxHeaderListSize: 1 << 30, + t: &Transport{PseudoHeaderOrder: tt.pseudoOrder}, + } + cc.henc = hpack.NewEncoder(&cc.hbuf) + + cc.mu.Lock() + hdrs, err := cc.encodeHeaders(req, tt.addGzipHeader, tt.trailers, tt.contentLength) + cc.mu.Unlock() + if err != nil { + t.Fatalf("encodeHeaders: %v", err) + } + + var got []string + dec := hpack.NewDecoder(initialHeaderTableSize, func(f hpack.HeaderField) { + got = append(got, f.Name+": "+f.Value) + }) + if _, err := dec.Write(hdrs); err != nil { + t.Fatalf("hpack decode: %v", err) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("emitted headers:\n got: %q\nwant: %q", got, tt.want) + } + }) + } +} + +func BenchmarkClientConnEncodeHeaders(b *testing.B) { + run := func(b *testing.B, h http.Header) { + b.ReportAllocs() + u, err := url.Parse("https://www.example.org/some/path?q=1") + if err != nil { + b.Fatal(err) + } + req := &http.Request{Method: "GET", URL: u, Header: h} + cc := &ClientConn{peerMaxHeaderListSize: 10 << 20, t: &Transport{}} + cc.henc = hpack.NewEncoder(&cc.hbuf) + cc.mu.Lock() + defer cc.mu.Unlock() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := cc.encodeHeaders(req, true, "", -1); err != nil { + b.Fatal(err) + } + } + } + + b.Run("Ordered", func(b *testing.B) { + run(b, http.Header{ + "sec-ch-ua": {"\"Chromium\";v=\"124\", \"Google Chrome\";v=\"124\""}, + "accept": {"*/*"}, + "x-requested-with": {"XMLHttpRequest"}, + "sec-ch-ua-mobile": {"?0"}, + "user-agent": {"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"}, + "content-type": {"application/json"}, + "origin": {"https://www.example.org"}, + "sec-fetch-site": {"same-origin"}, + "sec-fetch-mode": {"cors"}, + "sec-fetch-dest": {"empty"}, + "accept-language": {"en-US,en;q=0.9"}, + "accept-encoding": {"gzip, deflate, br"}, + "referer": {"https://www.example.org/x"}, + "cookie": {"a=1; b=2; c=3"}, + http.HeaderOrderKey: { + "sec-ch-ua", "accept", "x-requested-with", "sec-ch-ua-mobile", + "user-agent", "content-type", "origin", "sec-fetch-site", + "sec-fetch-mode", "sec-fetch-dest", "referer", "cookie", + "accept-encoding", "accept-language", + }, + http.PHeaderOrderKey: {":method", ":authority", ":scheme", ":path"}, + }) + }) + + b.Run("Unordered", func(b *testing.B) { + run(b, http.Header{ + "Accept": {"*/*"}, + "User-Agent": {"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"}, + "Content-Type": {"application/json"}, + "Origin": {"https://www.example.org"}, + "Accept-Language": {"en-US,en;q=0.9"}, + "Accept-Encoding": {"gzip, deflate, br"}, + "Referer": {"https://www.example.org/x"}, + "Cookie": {"a=1; b=2; c=3"}, + }) + }) +} + func activeStreams(cc *ClientConn) int { cc.mu.Lock() defer cc.mu.Unlock()