From 88aa71154855395304d857a3c20ffc5eb286c511 Mon Sep 17 00:00:00 2001 From: "antonio.alors" Date: Wed, 19 Aug 2026 08:14:29 +0000 Subject: [PATCH 1/5] transport/http: make inbound header preallocation configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Preserve the default post-system-header sizing from PR #2521 while giving header-heavy services an escape hatch for requests dominated by unrelated HTTP headers. - Let downstream services choose the CPU and memory tradeoff that matches their inbound header mix. Changes: - Added remaining, scan, and disabled inbound header preallocation strategies through the Go API and HTTP transport configuration. - Made the opt-in scan count unique application, tracing, proxy, and grabbed headers, including collision and minimum-threshold handling. - Precomputed grabbed-header lookup keys per inbound so scanning adds no per-request allocations, with focused tests and benchmarks for each strategy. Jira Issues: T3-RPC-11408 --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- transport/http/config.go | 29 +++++ transport/http/config_test.go | 51 +++++++++ transport/http/handler.go | 142 +++++++++++++++++++++++- transport/http/handler_test.go | 102 +++++++++++++++++ transport/http/header_benchmark_test.go | 13 ++- transport/http/inbound.go | 63 +++++++++-- transport/http/inbound_test.go | 35 ++++++ 7 files changed, 424 insertions(+), 11 deletions(-) diff --git a/transport/http/config.go b/transport/http/config.go index 31f14d7a1e..4c6b14bd9c 100644 --- a/transport/http/config.go +++ b/transport/http/config.go @@ -170,6 +170,7 @@ func (ts *transportSpec) buildTransport(tc *TransportConfig, k *yarpcconfig.Kit) // x-bar: // - X-Bar // - X-BAR +// headerPreallocation: scan // shutdownTimeout: 5s // readHeaderTimeout: 5s // readTimeout: 10s @@ -205,6 +206,10 @@ type InboundConfig struct { // Keys must be lowercase. Values are the desired original casings. // Ignored if CanonicalizeHeaderKeys is true. HeaderCaseMapping map[string][]string `config:"headerCaseMapping"` + // HeaderPreallocation controls how inbound transport header maps are + // preallocated. Supported values are "remaining" (default), "scan", and + // "disabled". + HeaderPreallocation string `config:"headerPreallocation"` } // TLSConfig specifies the TLS configuration of the HTTP inbound. @@ -275,11 +280,35 @@ func (ts *transportSpec) buildInbound(ic *InboundConfig, t transport.Transport, } } + if ic.HeaderPreallocation != "" { + strategy, err := parseHeaderPreallocationStrategy(ic.HeaderPreallocation) + if err != nil { + return nil, err + } + inboundOptions = append(inboundOptions, InboundHeaderPreallocation(strategy)) + } + inboundOptions = append(inboundOptions, DisableHTTP2(ic.DisableHTTP2)) return t.(*Transport).NewInbound(ic.Address, inboundOptions...), nil } +func parseHeaderPreallocationStrategy(value string) (HeaderPreallocationStrategy, error) { + switch value { + case "remaining": + return HeaderPreallocationRemaining, nil + case "scan": + return HeaderPreallocationScan, nil + case "disabled": + return HeaderPreallocationDisabled, nil + default: + return 0, fmt.Errorf( + "headerPreallocation must be one of remaining, scan, or disabled, got %q", + value, + ) + } +} + // OutboundConfig configures an HTTP outbound. // // outbounds: diff --git a/transport/http/config_test.go b/transport/http/config_test.go index 289a51dd01..31cb3f21c6 100644 --- a/transport/http/config_test.go +++ b/transport/http/config_test.go @@ -88,6 +88,7 @@ func TestTransportSpec(t *testing.T) { IdleTimeout time.Duration CanonicalizeHeaderKeys bool HeaderCaseMapping map[string][]string + HeaderPreallocation HeaderPreallocationStrategy } type inboundTest struct { @@ -349,6 +350,55 @@ func TestTransportSpec(t *testing.T) { CanonicalizeHeaderKeys: true, }, }, + { + desc: "header preallocation scan", + cfg: attrs{ + "address": ":8080", + "headerPreallocation": "scan", + }, + wantInbound: &wantInbound{ + Address: ":8080", + ShutdownTimeout: defaultShutdownTimeout, + HeaderPreallocation: HeaderPreallocationScan, + }, + }, + { + desc: "header preallocation disabled", + cfg: attrs{ + "address": ":8080", + "headerPreallocation": "disabled", + }, + wantInbound: &wantInbound{ + Address: ":8080", + ShutdownTimeout: defaultShutdownTimeout, + HeaderPreallocation: HeaderPreallocationDisabled, + }, + }, + { + desc: "header preallocation config overrides option", + cfg: attrs{ + "address": ":8080", + "headerPreallocation": "remaining", + }, + opts: []Option{ + InboundHeaderPreallocation(HeaderPreallocationScan), + }, + wantInbound: &wantInbound{ + Address: ":8080", + ShutdownTimeout: defaultShutdownTimeout, + HeaderPreallocation: HeaderPreallocationRemaining, + }, + }, + { + desc: "invalid header preallocation", + cfg: attrs{ + "address": ":8080", + "headerPreallocation": "unknown", + }, + wantErrors: []string{ + `headerPreallocation must be one of remaining, scan, or disabled, got "unknown"`, + }, + }, } outboundTests := []outboundTest{ @@ -696,6 +746,7 @@ func TestTransportSpec(t *testing.T) { assert.Equal(t, want.ReadTimeout, ib.server.ReadTimeout, "ReadTimeout should match") assert.Equal(t, want.IdleTimeout, ib.server.IdleTimeout, "IdleTimeout should match") assert.Equal(t, want.CanonicalizeHeaderKeys, ib.overrideOriginalItemWithCanonicalizedKey, "canonicalizeHeaderKeys should match") + assert.Equal(t, want.HeaderPreallocation, ib.headerPreallocationStrategy, "headerPreallocation should match") if len(want.HeaderCaseMapping) > 0 { assert.Equal(t, want.HeaderCaseMapping, ib.headerCaseMapping, "headerCaseMapping should match") } else { diff --git a/transport/http/handler.go b/transport/http/handler.go index 56f3ba7679..837bb04597 100644 --- a/transport/http/handler.go +++ b/transport/http/handler.go @@ -26,6 +26,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "time" "github.com/opentracing/opentracing-go" @@ -42,12 +43,145 @@ import ( "go.uber.org/zap" ) +const ( + // Preallocating for eight or fewer headers did not reduce allocations in + // benchmarks, so avoid paying for a scan on small eligible header sets. + _minInboundHeadersForPreallocation = 9 +) + func popHeader(h http.Header, n string) string { v := h.Get(n) h.Del(n) return v } +func inboundHeaderCapacity( + strategy HeaderPreallocationStrategy, + headers http.Header, + grabHeaders []headerPreallocationGrabHeader, +) int { + switch strategy { + case HeaderPreallocationRemaining: + return len(headers) + case HeaderPreallocationScan: + return scannedInboundHeaderCapacity(headers, grabHeaders) + case HeaderPreallocationDisabled: + return 0 + default: + // Inbounds validate the strategy before installing the handler. + return 0 + } +} + +type headerPreallocationGrabHeader struct { + key string + canonicalKey string + prefixedCanonicalKey string + prefixedLowerKey string +} + +func newHeaderPreallocationGrabHeaders( + grabHeaders map[string]struct{}, +) []headerPreallocationGrabHeader { + headers := make([]headerPreallocationGrabHeader, 0, len(grabHeaders)) + lowerApplicationHeaderPrefix := strings.ToLower(ApplicationHeaderPrefix) + for key := range grabHeaders { + headers = append(headers, headerPreallocationGrabHeader{ + key: key, + canonicalKey: http.CanonicalHeaderKey(key), + prefixedCanonicalKey: http.CanonicalHeaderKey(ApplicationHeaderPrefix + key), + prefixedLowerKey: lowerApplicationHeaderPrefix + key, + }) + } + return headers +} + +func scannedInboundHeaderCapacity( + headers http.Header, + grabHeaders []headerPreallocationGrabHeader, +) int { + capacity := 0 + for key, values := range headers { + if len(values) == 0 { + continue + } + + if hasPrefixFold(key, ApplicationHeaderPrefix) || + isTracingHeader(key) || + isProxyHeader(key) { + capacity++ + } + } + + for _, header := range grabHeaders { + if isTracingHeader(header.key) || isProxyHeader(header.key) { + continue + } + if headerValue(headers, header.canonicalKey, header.key) != "" && + !hasHeaderValues( + headers, + header.prefixedCanonicalKey, + header.prefixedLowerKey, + ) { + capacity++ + } + } + + // FromHTTPHeaders and the GrabHeaders loop can write the same canonical + // transport key through different HTTP header forms. Remove those + // collisions from the capacity estimate. + for key, values := range headers { + if len(values) == 0 || !hasPrefixFold(key, ApplicationHeaderPrefix) { + continue + } + + suffix := key[len(ApplicationHeaderPrefix):] + if (isTracingHeader(suffix) || isProxyHeader(suffix)) && + hasUnprefixedPropagatedHeader(headers, suffix) { + capacity-- + } + } + + if capacity < _minInboundHeadersForPreallocation { + return 0 + } + return capacity +} + +func hasUnprefixedPropagatedHeader(headers http.Header, key string) bool { + for candidate, values := range headers { + if len(values) > 0 && + !hasPrefixFold(candidate, ApplicationHeaderPrefix) && + strings.EqualFold(candidate, key) && + (isTracingHeader(candidate) || isProxyHeader(candidate)) { + return true + } + } + return false +} + +func headerValue(headers http.Header, canonicalKey, lowerKey string) string { + if values, ok := headers[canonicalKey]; ok { + if len(values) > 0 { + return values[0] + } + return "" + } + if lowerKey != canonicalKey { + if values := headers[lowerKey]; len(values) > 0 { + return values[0] + } + } + return "" +} + +func hasHeaderValues(headers http.Header, canonicalKey, lowerKey string) bool { + if values, ok := headers[canonicalKey]; ok { + return len(values) > 0 + } + return lowerKey != canonicalKey && len(headers[lowerKey]) > 0 +} + // handler adapts a transport.Handler into a handler for net/http. type handler struct { router transport.Router @@ -58,6 +192,8 @@ type handler struct { transport *Transport overrideOriginalItemWithCanonicalizedKey bool headerCaseMapping map[string][]string + headerPreallocationStrategy HeaderPreallocationStrategy + headerPreallocationGrabHeaders []headerPreallocationGrabHeader // duplicate header counter vector duplicateHeaderCounterVec *metrics.CounterVector } @@ -123,7 +259,11 @@ func (h handler) callHandler(responseWriter *responseWriter, req *http.Request, callerProcedure := popHeader(req.Header, CallerProcedureHeader) ttl := popHeader(req.Header, TTLMSHeader) - transportHeader := transport.NewHeadersWithCapacity(len(req.Header)) + transportHeader := transport.NewHeadersWithCapacity(inboundHeaderCapacity( + h.headerPreallocationStrategy, + req.Header, + h.headerPreallocationGrabHeaders, + )) if h.overrideOriginalItemWithCanonicalizedKey { transportHeader = transportHeader.EnableOverrideOriginalItemsWithCanonicalizedKeys() } diff --git a/transport/http/handler_test.go b/transport/http/handler_test.go index 190b05bc00..76c9507cf0 100644 --- a/transport/http/handler_test.go +++ b/transport/http/handler_test.go @@ -45,6 +45,108 @@ import ( "go.uber.org/yarpc/yarpcerrors" ) +func TestInboundHeaderCapacity(t *testing.T) { + t.Run("remaining", func(t *testing.T) { + headers := makeInboundPreallocationTestHeaders(2, 3) + assert.Equal(t, len(headers), inboundHeaderCapacity( + HeaderPreallocationRemaining, + headers, + nil, + )) + }) + + t.Run("disabled", func(t *testing.T) { + assert.Zero(t, inboundHeaderCapacity( + HeaderPreallocationDisabled, + makeInboundPreallocationTestHeaders(20, 0), + nil, + )) + }) + + t.Run("scan applies threshold to eligible headers", func(t *testing.T) { + headers := makeInboundPreallocationTestHeaders(8, 20) + assert.Zero(t, inboundHeaderCapacity( + HeaderPreallocationScan, + headers, + nil, + )) + }) + + t.Run("scan supports lowercase HTTP2 headers", func(t *testing.T) { + headers := make(http.Header, 10) + for i := 0; i < 7; i++ { + headers[fmt.Sprintf("rpc-header-application-%d", i)] = []string{"value"} + } + headers["rpc-header-x-grabbed"] = []string{"prefixed"} + headers["x-grabbed"] = []string{"raw"} + headers[XForwardedForHeader] = []string{"203.0.113.42"} + + grabHeaders := map[string]struct{}{ + "x-grabbed": {}, + XForwardedForHeader: {}, + } + assert.Equal(t, 9, inboundHeaderCapacity( + HeaderPreallocationScan, + headers, + newHeaderPreallocationGrabHeaders(grabHeaders), + )) + }) + + t.Run("scan counts propagated categories without collisions", func(t *testing.T) { + headers := makeInboundPreallocationTestHeaders(9, 1) + headers.Set(UberTraceContextHeaderKey, "trace") + headers.Set(UberBaggageHeaderKeyPrefix+"test", "baggage") + + proxyHeaders := map[string]string{ + XForwardedForHeader: "203.0.113.42", + XForwardedProtoHeader: "https", + XForwardedPortHeader: "443", + XRequestIDHeader: "request-id", + XUberSourceHeader: "source", + ViaHeader: "1.1 proxy", + UserAgentHeader: "yarpc/1.0", + } + for key, value := range proxyHeaders { + headers.Set(key, value) + } + + // Both forms produce one canonical transport header. + headers.Set(ApplicationHeaderPrefix+XForwardedForHeader, "prefixed-proxy") + headers.Set("X-Grabbed", "raw-grabbed") + headers.Set(ApplicationHeaderPrefix+"X-Grabbed", "prefixed-grabbed") + headers.Set("X-Grabbed-Only", "raw-grabbed") + headers.Set("X-Empty", "") + headers["Rpc-Header-No-Values"] = nil + + grabHeaders := map[string]struct{}{ + XForwardedForHeader: {}, + "x-grabbed": {}, + "x-grabbed-only": {}, + "x-empty": {}, + } + + assert.Equal(t, 20, inboundHeaderCapacity( + HeaderPreallocationScan, + headers, + newHeaderPreallocationGrabHeaders(grabHeaders), + )) + }) +} + +func makeInboundPreallocationTestHeaders(application, ignored int) http.Header { + headers := make(http.Header, application+ignored) + for i := 0; i < application; i++ { + headers.Set( + fmt.Sprintf("%sApplication-%d", ApplicationHeaderPrefix, i), + "application-value", + ) + } + for i := 0; i < ignored; i++ { + headers.Set(fmt.Sprintf("X-Ignored-%d", i), "ignored-value") + } + return headers +} + func TestHandlerSuccess(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() diff --git a/transport/http/header_benchmark_test.go b/transport/http/header_benchmark_test.go index 1bc7ba838f..2b6ac49dab 100644 --- a/transport/http/header_benchmark_test.go +++ b/transport/http/header_benchmark_test.go @@ -119,6 +119,7 @@ func BenchmarkFromHTTPHeadersCapacity(b *testing.B) { scenario.tracingHeaders + scenario.proxyHeaders + scenario.grabbedHeaders + preallocationGrabHeaders := newHeaderPreallocationGrabHeaders(grabHeaders) for _, mode := range scenario.modes { mode := mode @@ -135,9 +136,19 @@ func BenchmarkFromHTTPHeadersCapacity(b *testing.B) { }, }, { - name: "automatic", + name: "remaining", capacity: func() int { return len(from) }, }, + { + name: "scan", + capacity: func() int { + return inboundHeaderCapacity( + HeaderPreallocationScan, + from, + preallocationGrabHeaders, + ) + }, + }, } for _, benchmark := range benchmarks { diff --git a/transport/http/inbound.go b/transport/http/inbound.go index 7cc0546514..40e1b5e633 100644 --- a/transport/http/inbound.go +++ b/transport/http/inbound.go @@ -57,6 +57,24 @@ type InboundOption func(*Inbound) func (InboundOption) httpOption() {} +// HeaderPreallocationStrategy controls how an HTTP inbound sizes the +// transport header maps built for each request. +type HeaderPreallocationStrategy uint8 + +const ( + // HeaderPreallocationRemaining uses the number of HTTP headers remaining + // after YARPC system headers are removed. This is the default. + HeaderPreallocationRemaining HeaderPreallocationStrategy = iota + + // HeaderPreallocationScan scans the remaining HTTP headers and preallocates + // only for headers that YARPC will propagate. + HeaderPreallocationScan + + // HeaderPreallocationDisabled disables inbound transport header + // preallocation. + HeaderPreallocationDisabled +) + // Mux specifies that the HTTP server should make the YARPC endpoint available // under the given pattern on the given ServeMux. By default, the YARPC // service is made available on all paths of the HTTP server. By specifying a @@ -156,6 +174,20 @@ func HeaderCaseMapping(mapping map[string][]string) InboundOption { } } +// InboundHeaderPreallocation returns an InboundOption that controls how the +// inbound sizes the two transport header maps built for each request. +// +// HeaderPreallocationRemaining preserves the default behavior and uses the +// number of HTTP headers remaining after YARPC system headers are removed. +// HeaderPreallocationScan performs an additional scan to count only headers +// that YARPC will propagate, and HeaderPreallocationDisabled skips +// preallocation. +func InboundHeaderPreallocation(strategy HeaderPreallocationStrategy) InboundOption { + return func(i *Inbound) { + i.headerPreallocationStrategy = strategy + } +} + // ReadHeaderTimeout returns an InboundOption that sets the http.Server ReadHeaderTimeout func ReadHeaderTimeout(timeout time.Duration) InboundOption { return func(i *Inbound) { @@ -188,15 +220,16 @@ func IdleTimeout(timeout time.Duration) InboundOption { // sharing this transport. func (t *Transport) NewInbound(addr string, opts ...InboundOption) *Inbound { i := &Inbound{ - once: lifecycle.NewOnce(), - addr: addr, - shutdownTimeout: defaultShutdownTimeout, - tracer: t.tracer, - logger: t.logger, - transport: t, - grabHeaders: make(map[string]struct{}), - bothResponseError: true, - disableHTTP2: false, + once: lifecycle.NewOnce(), + addr: addr, + shutdownTimeout: defaultShutdownTimeout, + tracer: t.tracer, + logger: t.logger, + transport: t, + grabHeaders: make(map[string]struct{}), + bothResponseError: true, + disableHTTP2: false, + headerPreallocationStrategy: HeaderPreallocationRemaining, } server := &http.Server{ Addr: i.addr, @@ -235,6 +268,7 @@ type Inbound struct { disableHTTP2 bool overrideOriginalItemWithCanonicalizedKey bool headerCaseMapping map[string][]string + headerPreallocationStrategy HeaderPreallocationStrategy } // Tracer configures a tracer on this inbound. @@ -265,6 +299,15 @@ func (i *Inbound) start() error { if i.router == nil { return yarpcerrors.Newf(yarpcerrors.CodeInternal, "no router configured for transport inbound") } + switch i.headerPreallocationStrategy { + case HeaderPreallocationRemaining, HeaderPreallocationScan, HeaderPreallocationDisabled: + default: + return yarpcerrors.Newf( + yarpcerrors.CodeInvalidArgument, + "unknown header preallocation strategy: %d", + i.headerPreallocationStrategy, + ) + } for header := range i.grabHeaders { if !strings.HasPrefix(header, "x-") { return yarpcerrors.Newf(yarpcerrors.CodeInvalidArgument, "header %s does not begin with 'x-'", header) @@ -297,6 +340,8 @@ func (i *Inbound) start() error { logger: i.logger, overrideOriginalItemWithCanonicalizedKey: i.overrideOriginalItemWithCanonicalizedKey, headerCaseMapping: i.headerCaseMapping, + headerPreallocationStrategy: i.headerPreallocationStrategy, + headerPreallocationGrabHeaders: newHeaderPreallocationGrabHeaders(i.grabHeaders), duplicateHeaderCounterVec: duplicateHeaderCounterVec, } diff --git a/transport/http/inbound_test.go b/transport/http/inbound_test.go index 12d6a552d2..54af2c6305 100644 --- a/transport/http/inbound_test.go +++ b/transport/http/inbound_test.go @@ -110,6 +110,41 @@ func TestInboundStartErrorBadGrabHeader(t *testing.T) { assert.Equal(t, yarpcerrors.CodeInvalidArgument, yarpcerrors.FromError(i.Start()).Code()) } +func TestInboundHeaderPreallocation(t *testing.T) { + tests := []struct { + name string + strategy HeaderPreallocationStrategy + }{ + {name: "remaining", strategy: HeaderPreallocationRemaining}, + {name: "scan", strategy: HeaderPreallocationScan}, + {name: "disabled", strategy: HeaderPreallocationDisabled}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + x := NewTransport() + i := x.NewInbound( + "127.0.0.1:0", + InboundHeaderPreallocation(tt.strategy), + ) + assert.Equal(t, tt.strategy, i.headerPreallocationStrategy) + }) + } +} + +func TestInboundStartErrorInvalidHeaderPreallocation(t *testing.T) { + x := NewTransport() + i := x.NewInbound( + "127.0.0.1:0", + InboundHeaderPreallocation(HeaderPreallocationStrategy(255)), + ) + i.SetRouter(new(transporttest.MockRouter)) + + err := i.Start() + assert.Equal(t, yarpcerrors.CodeInvalidArgument, yarpcerrors.FromError(err).Code()) + assert.Contains(t, err.Error(), "unknown header preallocation strategy") +} + func TestInboundStopWithoutStarting(t *testing.T) { x := NewTransport() i := x.NewInbound("127.0.0.1:8000") From eb72adfb3628a31a9902b56e99360cf104caff34 Mon Sep 17 00:00:00 2001 From: "antonio.alors" Date: Wed, 19 Aug 2026 09:27:58 +0000 Subject: [PATCH 2/5] transport/http: clarify header preallocation strategies --- transport/http/config.go | 8 ++++---- transport/http/config_test.go | 6 +++--- transport/http/handler.go | 11 +---------- transport/http/handler_test.go | 8 ++++---- transport/http/header_benchmark_test.go | 2 +- transport/http/inbound.go | 12 ++++++------ transport/http/inbound_test.go | 2 +- 7 files changed, 20 insertions(+), 29 deletions(-) diff --git a/transport/http/config.go b/transport/http/config.go index 4c6b14bd9c..669462bd8d 100644 --- a/transport/http/config.go +++ b/transport/http/config.go @@ -207,7 +207,7 @@ type InboundConfig struct { // Ignored if CanonicalizeHeaderKeys is true. HeaderCaseMapping map[string][]string `config:"headerCaseMapping"` // HeaderPreallocation controls how inbound transport header maps are - // preallocated. Supported values are "remaining" (default), "scan", and + // preallocated. Supported values are "unfiltered" (default), "scan", and // "disabled". HeaderPreallocation string `config:"headerPreallocation"` } @@ -295,15 +295,15 @@ func (ts *transportSpec) buildInbound(ic *InboundConfig, t transport.Transport, func parseHeaderPreallocationStrategy(value string) (HeaderPreallocationStrategy, error) { switch value { - case "remaining": - return HeaderPreallocationRemaining, nil + case "unfiltered": + return HeaderPreallocationUnfiltered, nil case "scan": return HeaderPreallocationScan, nil case "disabled": return HeaderPreallocationDisabled, nil default: return 0, fmt.Errorf( - "headerPreallocation must be one of remaining, scan, or disabled, got %q", + "headerPreallocation must be one of unfiltered, scan, or disabled, got %q", value, ) } diff --git a/transport/http/config_test.go b/transport/http/config_test.go index 31cb3f21c6..43a7f0e79e 100644 --- a/transport/http/config_test.go +++ b/transport/http/config_test.go @@ -378,7 +378,7 @@ func TestTransportSpec(t *testing.T) { desc: "header preallocation config overrides option", cfg: attrs{ "address": ":8080", - "headerPreallocation": "remaining", + "headerPreallocation": "unfiltered", }, opts: []Option{ InboundHeaderPreallocation(HeaderPreallocationScan), @@ -386,7 +386,7 @@ func TestTransportSpec(t *testing.T) { wantInbound: &wantInbound{ Address: ":8080", ShutdownTimeout: defaultShutdownTimeout, - HeaderPreallocation: HeaderPreallocationRemaining, + HeaderPreallocation: HeaderPreallocationUnfiltered, }, }, { @@ -396,7 +396,7 @@ func TestTransportSpec(t *testing.T) { "headerPreallocation": "unknown", }, wantErrors: []string{ - `headerPreallocation must be one of remaining, scan, or disabled, got "unknown"`, + `headerPreallocation must be one of unfiltered, scan, or disabled, got "unknown"`, }, }, } diff --git a/transport/http/handler.go b/transport/http/handler.go index 837bb04597..d04d5c55bb 100644 --- a/transport/http/handler.go +++ b/transport/http/handler.go @@ -43,12 +43,6 @@ import ( "go.uber.org/zap" ) -const ( - // Preallocating for eight or fewer headers did not reduce allocations in - // benchmarks, so avoid paying for a scan on small eligible header sets. - _minInboundHeadersForPreallocation = 9 -) - func popHeader(h http.Header, n string) string { v := h.Get(n) h.Del(n) @@ -61,7 +55,7 @@ func inboundHeaderCapacity( grabHeaders []headerPreallocationGrabHeader, ) int { switch strategy { - case HeaderPreallocationRemaining: + case HeaderPreallocationUnfiltered: return len(headers) case HeaderPreallocationScan: return scannedInboundHeaderCapacity(headers, grabHeaders) @@ -142,9 +136,6 @@ func scannedInboundHeaderCapacity( } } - if capacity < _minInboundHeadersForPreallocation { - return 0 - } return capacity } diff --git a/transport/http/handler_test.go b/transport/http/handler_test.go index 76c9507cf0..2c4e3b09fe 100644 --- a/transport/http/handler_test.go +++ b/transport/http/handler_test.go @@ -46,10 +46,10 @@ import ( ) func TestInboundHeaderCapacity(t *testing.T) { - t.Run("remaining", func(t *testing.T) { + t.Run("unfiltered", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(2, 3) assert.Equal(t, len(headers), inboundHeaderCapacity( - HeaderPreallocationRemaining, + HeaderPreallocationUnfiltered, headers, nil, )) @@ -63,9 +63,9 @@ func TestInboundHeaderCapacity(t *testing.T) { )) }) - t.Run("scan applies threshold to eligible headers", func(t *testing.T) { + t.Run("scan uses eligible count for small header sets", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(8, 20) - assert.Zero(t, inboundHeaderCapacity( + assert.Equal(t, 8, inboundHeaderCapacity( HeaderPreallocationScan, headers, nil, diff --git a/transport/http/header_benchmark_test.go b/transport/http/header_benchmark_test.go index 2b6ac49dab..68ae357fa0 100644 --- a/transport/http/header_benchmark_test.go +++ b/transport/http/header_benchmark_test.go @@ -136,7 +136,7 @@ func BenchmarkFromHTTPHeadersCapacity(b *testing.B) { }, }, { - name: "remaining", + name: "unfiltered", capacity: func() int { return len(from) }, }, { diff --git a/transport/http/inbound.go b/transport/http/inbound.go index 40e1b5e633..8d4195e946 100644 --- a/transport/http/inbound.go +++ b/transport/http/inbound.go @@ -62,9 +62,9 @@ func (InboundOption) httpOption() {} type HeaderPreallocationStrategy uint8 const ( - // HeaderPreallocationRemaining uses the number of HTTP headers remaining - // after YARPC system headers are removed. This is the default. - HeaderPreallocationRemaining HeaderPreallocationStrategy = iota + // HeaderPreallocationUnfiltered uses the unfiltered number of HTTP headers + // remaining after YARPC system headers are removed. This is the default. + HeaderPreallocationUnfiltered HeaderPreallocationStrategy = iota // HeaderPreallocationScan scans the remaining HTTP headers and preallocates // only for headers that YARPC will propagate. @@ -177,7 +177,7 @@ func HeaderCaseMapping(mapping map[string][]string) InboundOption { // InboundHeaderPreallocation returns an InboundOption that controls how the // inbound sizes the two transport header maps built for each request. // -// HeaderPreallocationRemaining preserves the default behavior and uses the +// HeaderPreallocationUnfiltered preserves the default behavior and uses the // number of HTTP headers remaining after YARPC system headers are removed. // HeaderPreallocationScan performs an additional scan to count only headers // that YARPC will propagate, and HeaderPreallocationDisabled skips @@ -229,7 +229,7 @@ func (t *Transport) NewInbound(addr string, opts ...InboundOption) *Inbound { grabHeaders: make(map[string]struct{}), bothResponseError: true, disableHTTP2: false, - headerPreallocationStrategy: HeaderPreallocationRemaining, + headerPreallocationStrategy: HeaderPreallocationUnfiltered, } server := &http.Server{ Addr: i.addr, @@ -300,7 +300,7 @@ func (i *Inbound) start() error { return yarpcerrors.Newf(yarpcerrors.CodeInternal, "no router configured for transport inbound") } switch i.headerPreallocationStrategy { - case HeaderPreallocationRemaining, HeaderPreallocationScan, HeaderPreallocationDisabled: + case HeaderPreallocationUnfiltered, HeaderPreallocationScan, HeaderPreallocationDisabled: default: return yarpcerrors.Newf( yarpcerrors.CodeInvalidArgument, diff --git a/transport/http/inbound_test.go b/transport/http/inbound_test.go index 54af2c6305..7e8224c0e3 100644 --- a/transport/http/inbound_test.go +++ b/transport/http/inbound_test.go @@ -115,7 +115,7 @@ func TestInboundHeaderPreallocation(t *testing.T) { name string strategy HeaderPreallocationStrategy }{ - {name: "remaining", strategy: HeaderPreallocationRemaining}, + {name: "unfiltered", strategy: HeaderPreallocationUnfiltered}, {name: "scan", strategy: HeaderPreallocationScan}, {name: "disabled", strategy: HeaderPreallocationDisabled}, } From bdbf8aa3035b4d7ccf6f22ea1e73d8c0ffaf23cd Mon Sep 17 00:00:00 2001 From: "antonio.alors" Date: Mon, 24 Aug 2026 11:42:06 +0000 Subject: [PATCH 3/5] transport/http: type header preallocation config --- transport/http/config.go | 33 +++++++++---------------- transport/http/config_test.go | 12 +++++++++ transport/http/handler.go | 2 +- transport/http/handler_test.go | 9 +++++++ transport/http/inbound.go | 45 ++++++++++++++++++++-------------- transport/http/inbound_test.go | 3 ++- 6 files changed, 61 insertions(+), 43 deletions(-) diff --git a/transport/http/config.go b/transport/http/config.go index 669462bd8d..9eab59ae31 100644 --- a/transport/http/config.go +++ b/transport/http/config.go @@ -209,7 +209,7 @@ type InboundConfig struct { // HeaderPreallocation controls how inbound transport header maps are // preallocated. Supported values are "unfiltered" (default), "scan", and // "disabled". - HeaderPreallocation string `config:"headerPreallocation"` + HeaderPreallocation *HeaderPreallocationStrategy `config:"headerPreallocation"` } // TLSConfig specifies the TLS configuration of the HTTP inbound. @@ -280,12 +280,17 @@ func (ts *transportSpec) buildInbound(ic *InboundConfig, t transport.Transport, } } - if ic.HeaderPreallocation != "" { - strategy, err := parseHeaderPreallocationStrategy(ic.HeaderPreallocation) - if err != nil { - return nil, err + if ic.HeaderPreallocation != nil { + if !ic.HeaderPreallocation.valid() { + return nil, fmt.Errorf( + "headerPreallocation must be one of unfiltered, scan, or disabled, got %q", + *ic.HeaderPreallocation, + ) } - inboundOptions = append(inboundOptions, InboundHeaderPreallocation(strategy)) + inboundOptions = append( + inboundOptions, + InboundHeaderPreallocation(*ic.HeaderPreallocation), + ) } inboundOptions = append(inboundOptions, DisableHTTP2(ic.DisableHTTP2)) @@ -293,22 +298,6 @@ func (ts *transportSpec) buildInbound(ic *InboundConfig, t transport.Transport, return t.(*Transport).NewInbound(ic.Address, inboundOptions...), nil } -func parseHeaderPreallocationStrategy(value string) (HeaderPreallocationStrategy, error) { - switch value { - case "unfiltered": - return HeaderPreallocationUnfiltered, nil - case "scan": - return HeaderPreallocationScan, nil - case "disabled": - return HeaderPreallocationDisabled, nil - default: - return 0, fmt.Errorf( - "headerPreallocation must be one of unfiltered, scan, or disabled, got %q", - value, - ) - } -} - // OutboundConfig configures an HTTP outbound. // // outbounds: diff --git a/transport/http/config_test.go b/transport/http/config_test.go index 43a7f0e79e..c548ee3829 100644 --- a/transport/http/config_test.go +++ b/transport/http/config_test.go @@ -389,6 +389,18 @@ func TestTransportSpec(t *testing.T) { HeaderPreallocation: HeaderPreallocationUnfiltered, }, }, + { + desc: "header preallocation option without config", + cfg: attrs{"address": ":8080"}, + opts: []Option{ + InboundHeaderPreallocation(HeaderPreallocationScan), + }, + wantInbound: &wantInbound{ + Address: ":8080", + ShutdownTimeout: defaultShutdownTimeout, + HeaderPreallocation: HeaderPreallocationScan, + }, + }, { desc: "invalid header preallocation", cfg: attrs{ diff --git a/transport/http/handler.go b/transport/http/handler.go index d04d5c55bb..13043ed959 100644 --- a/transport/http/handler.go +++ b/transport/http/handler.go @@ -55,7 +55,7 @@ func inboundHeaderCapacity( grabHeaders []headerPreallocationGrabHeader, ) int { switch strategy { - case HeaderPreallocationUnfiltered: + case "", HeaderPreallocationUnfiltered: return len(headers) case HeaderPreallocationScan: return scannedInboundHeaderCapacity(headers, grabHeaders) diff --git a/transport/http/handler_test.go b/transport/http/handler_test.go index 2c4e3b09fe..55211b4b62 100644 --- a/transport/http/handler_test.go +++ b/transport/http/handler_test.go @@ -46,6 +46,15 @@ import ( ) func TestInboundHeaderCapacity(t *testing.T) { + t.Run("zero value uses unfiltered", func(t *testing.T) { + headers := makeInboundPreallocationTestHeaders(2, 3) + assert.Equal(t, len(headers), inboundHeaderCapacity( + HeaderPreallocationStrategy(""), + headers, + nil, + )) + }) + t.Run("unfiltered", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(2, 3) assert.Equal(t, len(headers), inboundHeaderCapacity( diff --git a/transport/http/inbound.go b/transport/http/inbound.go index 8d4195e946..6b39a8212d 100644 --- a/transport/http/inbound.go +++ b/transport/http/inbound.go @@ -58,23 +58,33 @@ type InboundOption func(*Inbound) func (InboundOption) httpOption() {} // HeaderPreallocationStrategy controls how an HTTP inbound sizes the -// transport header maps built for each request. -type HeaderPreallocationStrategy uint8 +// transport header maps built for each request. Its zero value uses the +// unfiltered strategy. +type HeaderPreallocationStrategy string const ( // HeaderPreallocationUnfiltered uses the unfiltered number of HTTP headers // remaining after YARPC system headers are removed. This is the default. - HeaderPreallocationUnfiltered HeaderPreallocationStrategy = iota + HeaderPreallocationUnfiltered HeaderPreallocationStrategy = "unfiltered" // HeaderPreallocationScan scans the remaining HTTP headers and preallocates // only for headers that YARPC will propagate. - HeaderPreallocationScan + HeaderPreallocationScan HeaderPreallocationStrategy = "scan" // HeaderPreallocationDisabled disables inbound transport header // preallocation. - HeaderPreallocationDisabled + HeaderPreallocationDisabled HeaderPreallocationStrategy = "disabled" ) +func (s HeaderPreallocationStrategy) valid() bool { + switch s { + case "", HeaderPreallocationUnfiltered, HeaderPreallocationScan, HeaderPreallocationDisabled: + return true + default: + return false + } +} + // Mux specifies that the HTTP server should make the YARPC endpoint available // under the given pattern on the given ServeMux. By default, the YARPC // service is made available on all paths of the HTTP server. By specifying a @@ -220,16 +230,15 @@ func IdleTimeout(timeout time.Duration) InboundOption { // sharing this transport. func (t *Transport) NewInbound(addr string, opts ...InboundOption) *Inbound { i := &Inbound{ - once: lifecycle.NewOnce(), - addr: addr, - shutdownTimeout: defaultShutdownTimeout, - tracer: t.tracer, - logger: t.logger, - transport: t, - grabHeaders: make(map[string]struct{}), - bothResponseError: true, - disableHTTP2: false, - headerPreallocationStrategy: HeaderPreallocationUnfiltered, + once: lifecycle.NewOnce(), + addr: addr, + shutdownTimeout: defaultShutdownTimeout, + tracer: t.tracer, + logger: t.logger, + transport: t, + grabHeaders: make(map[string]struct{}), + bothResponseError: true, + disableHTTP2: false, } server := &http.Server{ Addr: i.addr, @@ -299,12 +308,10 @@ func (i *Inbound) start() error { if i.router == nil { return yarpcerrors.Newf(yarpcerrors.CodeInternal, "no router configured for transport inbound") } - switch i.headerPreallocationStrategy { - case HeaderPreallocationUnfiltered, HeaderPreallocationScan, HeaderPreallocationDisabled: - default: + if !i.headerPreallocationStrategy.valid() { return yarpcerrors.Newf( yarpcerrors.CodeInvalidArgument, - "unknown header preallocation strategy: %d", + "unknown header preallocation strategy: %q", i.headerPreallocationStrategy, ) } diff --git a/transport/http/inbound_test.go b/transport/http/inbound_test.go index 7e8224c0e3..c13177955e 100644 --- a/transport/http/inbound_test.go +++ b/transport/http/inbound_test.go @@ -115,6 +115,7 @@ func TestInboundHeaderPreallocation(t *testing.T) { name string strategy HeaderPreallocationStrategy }{ + {name: "zero value", strategy: HeaderPreallocationStrategy("")}, {name: "unfiltered", strategy: HeaderPreallocationUnfiltered}, {name: "scan", strategy: HeaderPreallocationScan}, {name: "disabled", strategy: HeaderPreallocationDisabled}, @@ -136,7 +137,7 @@ func TestInboundStartErrorInvalidHeaderPreallocation(t *testing.T) { x := NewTransport() i := x.NewInbound( "127.0.0.1:0", - InboundHeaderPreallocation(HeaderPreallocationStrategy(255)), + InboundHeaderPreallocation(HeaderPreallocationStrategy("unknown")), ) i.SetRouter(new(transporttest.MockRouter)) From 99eea9f3b5446f01fd55ee8f3e0c6599722b7d72 Mon Sep 17 00:00:00 2001 From: "antonio.alors" Date: Wed, 26 Aug 2026 10:10:07 +0000 Subject: [PATCH 4/5] transport/http: allow small collision overallocations --- transport/http/handler.go | 58 +++++----------------------------- transport/http/handler_test.go | 10 +++--- transport/http/inbound.go | 9 +++--- 3 files changed, 19 insertions(+), 58 deletions(-) diff --git a/transport/http/handler.go b/transport/http/handler.go index 13043ed959..84d99dd434 100644 --- a/transport/http/handler.go +++ b/transport/http/handler.go @@ -26,7 +26,6 @@ import ( "fmt" "net/http" "strconv" - "strings" "time" "github.com/opentracing/opentracing-go" @@ -68,23 +67,18 @@ func inboundHeaderCapacity( } type headerPreallocationGrabHeader struct { - key string - canonicalKey string - prefixedCanonicalKey string - prefixedLowerKey string + key string + canonicalKey string } func newHeaderPreallocationGrabHeaders( grabHeaders map[string]struct{}, ) []headerPreallocationGrabHeader { headers := make([]headerPreallocationGrabHeader, 0, len(grabHeaders)) - lowerApplicationHeaderPrefix := strings.ToLower(ApplicationHeaderPrefix) for key := range grabHeaders { headers = append(headers, headerPreallocationGrabHeader{ - key: key, - canonicalKey: http.CanonicalHeaderKey(key), - prefixedCanonicalKey: http.CanonicalHeaderKey(ApplicationHeaderPrefix + key), - prefixedLowerKey: lowerApplicationHeaderPrefix + key, + key: key, + canonicalKey: http.CanonicalHeaderKey(key), }) } return headers @@ -111,46 +105,17 @@ func scannedInboundHeaderCapacity( if isTracingHeader(header.key) || isProxyHeader(header.key) { continue } - if headerValue(headers, header.canonicalKey, header.key) != "" && - !hasHeaderValues( - headers, - header.prefixedCanonicalKey, - header.prefixedLowerKey, - ) { + if headerValue(headers, header.canonicalKey, header.key) != "" { capacity++ } } - // FromHTTPHeaders and the GrabHeaders loop can write the same canonical - // transport key through different HTTP header forms. Remove those - // collisions from the capacity estimate. - for key, values := range headers { - if len(values) == 0 || !hasPrefixFold(key, ApplicationHeaderPrefix) { - continue - } - - suffix := key[len(ApplicationHeaderPrefix):] - if (isTracingHeader(suffix) || isProxyHeader(suffix)) && - hasUnprefixedPropagatedHeader(headers, suffix) { - capacity-- - } - } - + // Raw and prefixed forms can produce the same canonical transport key. + // Count both forms: the estimate is an upper bound for items and may be + // required by originalItems, which preserves both original key forms. return capacity } -func hasUnprefixedPropagatedHeader(headers http.Header, key string) bool { - for candidate, values := range headers { - if len(values) > 0 && - !hasPrefixFold(candidate, ApplicationHeaderPrefix) && - strings.EqualFold(candidate, key) && - (isTracingHeader(candidate) || isProxyHeader(candidate)) { - return true - } - } - return false -} - func headerValue(headers http.Header, canonicalKey, lowerKey string) string { if values, ok := headers[canonicalKey]; ok { if len(values) > 0 { @@ -166,13 +131,6 @@ func headerValue(headers http.Header, canonicalKey, lowerKey string) string { return "" } -func hasHeaderValues(headers http.Header, canonicalKey, lowerKey string) bool { - if values, ok := headers[canonicalKey]; ok { - return len(values) > 0 - } - return lowerKey != canonicalKey && len(headers[lowerKey]) > 0 -} - // handler adapts a transport.Handler into a handler for net/http. type handler struct { router transport.Router diff --git a/transport/http/handler_test.go b/transport/http/handler_test.go index 55211b4b62..de734a0186 100644 --- a/transport/http/handler_test.go +++ b/transport/http/handler_test.go @@ -94,14 +94,15 @@ func TestInboundHeaderCapacity(t *testing.T) { "x-grabbed": {}, XForwardedForHeader: {}, } - assert.Equal(t, 9, inboundHeaderCapacity( + // Count both raw and prefixed grabbed forms. + assert.Equal(t, 10, inboundHeaderCapacity( HeaderPreallocationScan, headers, newHeaderPreallocationGrabHeaders(grabHeaders), )) }) - t.Run("scan counts propagated categories without collisions", func(t *testing.T) { + t.Run("scan allows small overestimate for duplicate forms", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(9, 1) headers.Set(UberTraceContextHeaderKey, "trace") headers.Set(UberBaggageHeaderKeyPrefix+"test", "baggage") @@ -119,7 +120,8 @@ func TestInboundHeaderCapacity(t *testing.T) { headers.Set(key, value) } - // Both forms produce one canonical transport header. + // Both pairs produce one canonical item; the grabbed pair also preserves + // two differently-cased original items. headers.Set(ApplicationHeaderPrefix+XForwardedForHeader, "prefixed-proxy") headers.Set("X-Grabbed", "raw-grabbed") headers.Set(ApplicationHeaderPrefix+"X-Grabbed", "prefixed-grabbed") @@ -134,7 +136,7 @@ func TestInboundHeaderCapacity(t *testing.T) { "x-empty": {}, } - assert.Equal(t, 20, inboundHeaderCapacity( + assert.Equal(t, 22, inboundHeaderCapacity( HeaderPreallocationScan, headers, newHeaderPreallocationGrabHeaders(grabHeaders), diff --git a/transport/http/inbound.go b/transport/http/inbound.go index 6b39a8212d..951360121b 100644 --- a/transport/http/inbound.go +++ b/transport/http/inbound.go @@ -68,7 +68,8 @@ const ( HeaderPreallocationUnfiltered HeaderPreallocationStrategy = "unfiltered" // HeaderPreallocationScan scans the remaining HTTP headers and preallocates - // only for headers that YARPC will propagate. + // for header forms that YARPC will propagate. Duplicate raw and prefixed + // forms may cause a small overestimate. HeaderPreallocationScan HeaderPreallocationStrategy = "scan" // HeaderPreallocationDisabled disables inbound transport header @@ -189,9 +190,9 @@ func HeaderCaseMapping(mapping map[string][]string) InboundOption { // // HeaderPreallocationUnfiltered preserves the default behavior and uses the // number of HTTP headers remaining after YARPC system headers are removed. -// HeaderPreallocationScan performs an additional scan to count only headers -// that YARPC will propagate, and HeaderPreallocationDisabled skips -// preallocation. +// HeaderPreallocationScan performs an additional scan to count header forms +// that YARPC will propagate. Duplicate raw and prefixed forms may cause a small +// overestimate. HeaderPreallocationDisabled skips preallocation. func InboundHeaderPreallocation(strategy HeaderPreallocationStrategy) InboundOption { return func(i *Inbound) { i.headerPreallocationStrategy = strategy From f0966342df16c885f3bcbf41b66f7249321f04c6 Mon Sep 17 00:00:00 2001 From: "antonio.alors" Date: Wed, 26 Aug 2026 13:55:12 +0000 Subject: [PATCH 5/5] transport/http: make preallocation default explicit --- transport/http/config_test.go | 16 +++++++++++++++- transport/http/handler.go | 17 +++++++---------- transport/http/handler_test.go | 25 ++++++++++++++++--------- transport/http/inbound.go | 26 +++++++++++++------------- transport/http/inbound_test.go | 34 ++++++++++++++++++++++++---------- 5 files changed, 75 insertions(+), 43 deletions(-) diff --git a/transport/http/config_test.go b/transport/http/config_test.go index c548ee3829..7df9448498 100644 --- a/transport/http/config_test.go +++ b/transport/http/config_test.go @@ -411,6 +411,16 @@ func TestTransportSpec(t *testing.T) { `headerPreallocation must be one of unfiltered, scan, or disabled, got "unknown"`, }, }, + { + desc: "empty header preallocation", + cfg: attrs{ + "address": ":8080", + "headerPreallocation": "", + }, + wantErrors: []string{ + `headerPreallocation must be one of unfiltered, scan, or disabled, got ""`, + }, + }, } outboundTests := []outboundTest{ @@ -758,7 +768,11 @@ func TestTransportSpec(t *testing.T) { assert.Equal(t, want.ReadTimeout, ib.server.ReadTimeout, "ReadTimeout should match") assert.Equal(t, want.IdleTimeout, ib.server.IdleTimeout, "IdleTimeout should match") assert.Equal(t, want.CanonicalizeHeaderKeys, ib.overrideOriginalItemWithCanonicalizedKey, "canonicalizeHeaderKeys should match") - assert.Equal(t, want.HeaderPreallocation, ib.headerPreallocationStrategy, "headerPreallocation should match") + wantHeaderPreallocation := want.HeaderPreallocation + if wantHeaderPreallocation == "" { + wantHeaderPreallocation = HeaderPreallocationUnfiltered + } + assert.Equal(t, wantHeaderPreallocation, ib.headerPreallocationStrategy, "headerPreallocation should match") if len(want.HeaderCaseMapping) > 0 { assert.Equal(t, want.HeaderCaseMapping, ib.headerCaseMapping, "headerCaseMapping should match") } else { diff --git a/transport/http/handler.go b/transport/http/handler.go index 84d99dd434..243bd5679d 100644 --- a/transport/http/handler.go +++ b/transport/http/handler.go @@ -54,14 +54,15 @@ func inboundHeaderCapacity( grabHeaders []headerPreallocationGrabHeader, ) int { switch strategy { - case "", HeaderPreallocationUnfiltered: + case HeaderPreallocationUnfiltered: return len(headers) case HeaderPreallocationScan: return scannedInboundHeaderCapacity(headers, grabHeaders) case HeaderPreallocationDisabled: return 0 default: - // Inbounds validate the strategy before installing the handler. + // Unreachable: inbounds validate the strategy before installing the + // handler. return 0 } } @@ -89,6 +90,8 @@ func scannedInboundHeaderCapacity( grabHeaders []headerPreallocationGrabHeader, ) int { capacity := 0 + // Count eligible wire forms independently to keep the scan linear. + // Raw and prefixed forms may therefore slightly overestimate capacity. for key, values := range headers { if len(values) == 0 { continue @@ -110,18 +113,12 @@ func scannedInboundHeaderCapacity( } } - // Raw and prefixed forms can produce the same canonical transport key. - // Count both forms: the estimate is an upper bound for items and may be - // required by originalItems, which preserves both original key forms. return capacity } func headerValue(headers http.Header, canonicalKey, lowerKey string) string { - if values, ok := headers[canonicalKey]; ok { - if len(values) > 0 { - return values[0] - } - return "" + if values := headers[canonicalKey]; len(values) > 0 && values[0] != "" { + return values[0] } if lowerKey != canonicalKey { if values := headers[lowerKey]; len(values) > 0 { diff --git a/transport/http/handler_test.go b/transport/http/handler_test.go index de734a0186..7a55712aa0 100644 --- a/transport/http/handler_test.go +++ b/transport/http/handler_test.go @@ -46,15 +46,6 @@ import ( ) func TestInboundHeaderCapacity(t *testing.T) { - t.Run("zero value uses unfiltered", func(t *testing.T) { - headers := makeInboundPreallocationTestHeaders(2, 3) - assert.Equal(t, len(headers), inboundHeaderCapacity( - HeaderPreallocationStrategy(""), - headers, - nil, - )) - }) - t.Run("unfiltered", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(2, 3) assert.Equal(t, len(headers), inboundHeaderCapacity( @@ -102,6 +93,22 @@ func TestInboundHeaderCapacity(t *testing.T) { )) }) + t.Run("scan checks lowercase value after empty canonical value", func(t *testing.T) { + headers := http.Header{ + "X-Grabbed": {""}, + "x-grabbed": {"raw"}, + } + grabHeaders := map[string]struct{}{ + "x-grabbed": {}, + } + + assert.Equal(t, 1, inboundHeaderCapacity( + HeaderPreallocationScan, + headers, + newHeaderPreallocationGrabHeaders(grabHeaders), + )) + }) + t.Run("scan allows small overestimate for duplicate forms", func(t *testing.T) { headers := makeInboundPreallocationTestHeaders(9, 1) headers.Set(UberTraceContextHeaderKey, "trace") diff --git a/transport/http/inbound.go b/transport/http/inbound.go index 951360121b..3864c8cc5e 100644 --- a/transport/http/inbound.go +++ b/transport/http/inbound.go @@ -58,8 +58,7 @@ type InboundOption func(*Inbound) func (InboundOption) httpOption() {} // HeaderPreallocationStrategy controls how an HTTP inbound sizes the -// transport header maps built for each request. Its zero value uses the -// unfiltered strategy. +// transport header maps built for each request. type HeaderPreallocationStrategy string const ( @@ -73,13 +72,13 @@ const ( HeaderPreallocationScan HeaderPreallocationStrategy = "scan" // HeaderPreallocationDisabled disables inbound transport header - // preallocation. + // preallocation. Header maps are allocated lazily when first populated. HeaderPreallocationDisabled HeaderPreallocationStrategy = "disabled" ) func (s HeaderPreallocationStrategy) valid() bool { switch s { - case "", HeaderPreallocationUnfiltered, HeaderPreallocationScan, HeaderPreallocationDisabled: + case HeaderPreallocationUnfiltered, HeaderPreallocationScan, HeaderPreallocationDisabled: return true default: return false @@ -231,15 +230,16 @@ func IdleTimeout(timeout time.Duration) InboundOption { // sharing this transport. func (t *Transport) NewInbound(addr string, opts ...InboundOption) *Inbound { i := &Inbound{ - once: lifecycle.NewOnce(), - addr: addr, - shutdownTimeout: defaultShutdownTimeout, - tracer: t.tracer, - logger: t.logger, - transport: t, - grabHeaders: make(map[string]struct{}), - bothResponseError: true, - disableHTTP2: false, + once: lifecycle.NewOnce(), + addr: addr, + shutdownTimeout: defaultShutdownTimeout, + tracer: t.tracer, + logger: t.logger, + transport: t, + grabHeaders: make(map[string]struct{}), + bothResponseError: true, + disableHTTP2: false, + headerPreallocationStrategy: HeaderPreallocationUnfiltered, } server := &http.Server{ Addr: i.addr, diff --git a/transport/http/inbound_test.go b/transport/http/inbound_test.go index c13177955e..538f1541ce 100644 --- a/transport/http/inbound_test.go +++ b/transport/http/inbound_test.go @@ -111,11 +111,14 @@ func TestInboundStartErrorBadGrabHeader(t *testing.T) { } func TestInboundHeaderPreallocation(t *testing.T) { + x := NewTransport() + i := x.NewInbound("127.0.0.1:0") + assert.Equal(t, HeaderPreallocationUnfiltered, i.headerPreallocationStrategy) + tests := []struct { name string strategy HeaderPreallocationStrategy }{ - {name: "zero value", strategy: HeaderPreallocationStrategy("")}, {name: "unfiltered", strategy: HeaderPreallocationUnfiltered}, {name: "scan", strategy: HeaderPreallocationScan}, {name: "disabled", strategy: HeaderPreallocationDisabled}, @@ -134,16 +137,27 @@ func TestInboundHeaderPreallocation(t *testing.T) { } func TestInboundStartErrorInvalidHeaderPreallocation(t *testing.T) { - x := NewTransport() - i := x.NewInbound( - "127.0.0.1:0", - InboundHeaderPreallocation(HeaderPreallocationStrategy("unknown")), - ) - i.SetRouter(new(transporttest.MockRouter)) + tests := []struct { + name string + strategy HeaderPreallocationStrategy + }{ + {name: "empty", strategy: ""}, + {name: "unknown", strategy: "unknown"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + x := NewTransport() + i := x.NewInbound( + "127.0.0.1:0", + InboundHeaderPreallocation(tt.strategy), + ) + i.SetRouter(new(transporttest.MockRouter)) - err := i.Start() - assert.Equal(t, yarpcerrors.CodeInvalidArgument, yarpcerrors.FromError(err).Code()) - assert.Contains(t, err.Error(), "unknown header preallocation strategy") + err := i.Start() + assert.Equal(t, yarpcerrors.CodeInvalidArgument, yarpcerrors.FromError(err).Code()) + assert.Contains(t, err.Error(), "unknown header preallocation strategy") + }) + } } func TestInboundStopWithoutStarting(t *testing.T) {