fix: give net/http, Prometheus, OTel and slog copies of the request strings they keep, and give Adapt's request its headers (#732, #720) - #736
Conversation
…rings kept by net/http, Prometheus, OTel and slog survive the next request (#720, #732) Each test runs std, and epoll and io_uring with sync and async handlers. Requests with the same layout and different values go over one keep-alive connection (for the metrics cross-connection test, over a closed connection's pooled buffer), and what the API kept from an earlier request must still read that request: - TestAdaptRequestCarriesHeaders (#720): the adapted handler's request has the request headers, with nothing reading a header before Adapt. - TestAdaptKeptRequestStringsSurviveNextRequest, and TestWrapMiddlewareKeptStringsSurviveNextRequest in middleware/adapters: the method, path, query, Host, a header value and a header name net/http does not canonicalize, kept by the net/http handler or middleware. - TestLabelValuesSurviveNextRequest and TestLabelValuesSurviveConnectionReuse in middleware/metrics: the requests_total series labels (method, a LabelFuncs value), with no Gather error, and no label holding another connection's Authorization bytes. - TestAttributesSurviveNextRequest in middleware/otel: the span name and string attributes and the duration series attributes. - TestKeptRecordSurvivesNextRequest in middleware/logger: every string attribute of a record a handler keeps with Record.Clone. A CI step runs the seven tests by name with CELERIS_REQUIRE_IOURING_WORKERS=1 and an exact tally (7 tests, 35 arms, 14 of them io_uring, no SKIP), since the package steps run without -v and a test drops its io_uring arms silently when the probe finds no ring.
…ring (#720) buildHTTPRequest copied the headers from c.stream.Headers without calling MaterializeHeaders. The H1 parser of the native engines fills that slice only when something reads a header (populateCachedStream keeps the raw headers in LazyRawHeaders), so on a route with no header read before Adapt the adapted handler got a request with no headers: no Authorization, Cookie or Content-Type. This also affected adapters.ReverseProxy, which is built on Adapt and so forwarded no request headers. std fills the slice eagerly, and middleware/adapters reads it through RequestHeaders, which materializes it; neither was affected.
…s.WrapMiddleware (#732) net/http lets a handler or middleware keep the request's strings after ServeHTTP returns: a log line queued for later, a rate limiter's map key, a value handed to a goroutine. buildHTTPRequest (Adapt, AdaptFunc and adapters.ReverseProxy) and middleware/adapters.buildRequest (WrapMiddleware) built the request from the method, path, query, Host and header strings as they were: on epoll and io_uring, views of the connection's receive buffer, which the engine reuses for the connection's next request and, once the connection closes, for another connection. A kept string then read other request bytes. Both now copy every string they hand to net/http into one allocation per request, cut back out in order, so the URL (path and query) is one string and the "?" concatenation is gone. Header names are copied too: net/http canonicalizes a name by allocating a new string, except when it is already canonical (for example, a name with no letters), which it returns as it is. The body is not copied: net/http forbids reading it after ServeHTTP returns.
…er new label combination (#732) client_golang keeps the label values of every new series for the life of the registry and does not copy them. The middleware passed the request's strings: c.Method() for a method the H1 parser does not intern, c.Path() when there is no route pattern, and every LabelFuncs value (typically a header). On epoll and io_uring those are views of the connection's receive buffer, which the engine reuses for the connection's next request and, once the connection closes, for another connection, so a series' labels changed to other request bytes: Gather reported duplicate series, and a label read another client's Authorization header. (The label-value pool's comment said Prometheus does not retain the values; it keeps them when a series is new.) The middleware now keeps its own set of the label combinations it has recorded, keyed by the values (length-prefixed, built on the stack). A combination seen for the first time is copied once, into the map key, and the values handed to Prometheus are cut from that copy. The set also keeps the series it resolved: a request whose combination was seen before copies nothing and makes one map lookup instead of four WithLabelValues calls (each of which validates, hashes and looks up the values under the vector's lock). The request and response size histograms are resolved on first use, so a combination that never carried a body still has no size series. WithLabelValues runs outside the set's lock, since it panics on a label value that is not valid UTF-8.
…ings (#732) A span processor keeps an ended span until it exports it, and the metric SDK keeps every attribute set it has seen as an aggregation key for the life of the provider; neither copies strings. The middleware built them from the request's strings: url.path, server.address, user_agent.original, client.address, request.id, url.scheme, http.request.method_original, the span name when SpanNameFormatter is set or there is no route, and the CustomAttributes and CustomMetricAttributes values. On epoll and io_uring those are views of the connection's receive buffer, which the engine reuses for the connection's next request and, once the connection closes, for another connection, so a kept span or attribute set read other request bytes. The request strings are now copied into one allocation per request (ownStrings), and the span and metric attributes share the copies. normalizeMethod returns the package's own constant instead of its argument, so http.request.method for TRACE and CONNECT (standard, but not interned by the H1 parser) is no longer a view. String and string-slice values of the custom attributes are copied as they are appended; keys are not. Only otel.go and config.go change; carrier.go (the propagator carrier, in PR #723) is untouched.
…string values (#732) slog lets a Handler keep a Record after Handle returns by calling Record.Clone, which shares the strings; asynchronous and batching handlers do. The middleware logged the request's strings: method, path, Host, User-Agent, Referer, query, client IP, request ID, and whatever LogContextKeys, LogResponseHeaders and Fields values code derived from request headers. On epoll and io_uring those are views of the connection's receive buffer, which the engine reuses for the connection's next request and, once the connection closes, for another connection, so a kept record later formatted other request bytes. At the handler boundary, a handler other than the package's own (FastHandler and its WithGroup handler, which format the record before they return) now receives copies of every string value, descending into groups; the top-level copies share one allocation per logged request. FastHandler's path is unchanged and copies nothing.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pull request materializes headers and copies request-derived strings before adapted handlers and middleware can retain them. Linux tests check header availability and string retention across requests, connections, and supported engines. CI runs the retention tests with race detection. ChangesRequest String Retention
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is identified. The change is mergeable after normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Full details: Out of Scope Changes checkExplanation The changes in
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 2.12%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
Fixes #720. Refs #732: this fixes four of its sites; #742 tracks the rest (a fifth site in the recovery middleware, and otel
SLICE/MAPcustom attribute values), so #732 stays open.What was wrong
On epoll and io_uring, request strings (method, path, query, Host, header names and values, and whatever middleware derive from them) are views of the connection's receive buffer. The engine reuses that buffer for the connection's next request, and once the connection closes it pools the buffer and a newly accepted connection reads into it. Reading a view during the request is fine. Keeping one after the request is not, because the kept string later reads whatever the engine receives next.
Four sites handed these views to APIs that keep them (#732):
celeris.Adaptandadapters.WrapMiddlewarebuilt the*http.Requestfrom the views. net/http lets a handler or middleware keep the request's strings afterServeHTTPreturns, for example in a map key, a queued log line or a goroutine.c.Method()(for a method the H1 parser does not intern),c.Path()(when there is no route pattern) and everyLabelFuncsvalue toWithLabelValues. client_golang keeps the label values of every new series for the life of the registry. The series' labels then changed to later request bytes:Gatherreported duplicate series, and onerequests_totallabel readion: SECRET, bytes of another connection'sAuthorization: SECRET...header.url.path,server.address,user_agent.original,client.address,request.id,url.scheme,http.request.method_original, theCustomAttributesvalues, and the span name whenSpanNameFormatteris set) and metric attributes (server.address,url.scheme,CustomMetricAttributes) from the views.normalizeMethodalso returned its argument, sohttp.request.methodfor TRACE and CONNECT was a view too, since the H1 parser does not intern those methods. A span processor keeps an ended span until it exports it, and the metric SDK keeps every attribute set as an aggregation key.LogContextKeys,LogResponseHeadersandFieldsvalues derived from headers). slog lets a handler keep a record afterHandlereturns by callingRecord.Clone, which shares the strings. Asynchronous and batching handlers do this.Separately (#720),
buildHTTPRequestreadc.stream.Headerswithout callingMaterializeHeaders. The native H1 parser fills that slice only when something reads a header, so on a route with no earlier header read, the adapted handler got a request with no headers. This also meantadapters.ReverseProxy, which is built onAdapt, forwarded no request headers on epoll and io_uring. std andadapters.WrapMiddlewarewere not affected.The fix
Adapt(bridge.go)c.stream.MaterializeHeaders()before the header loop (#720). Method, URL, header names and values, and Host are copied into one buffer and cut back out in order."?"concatenation is gone, because path and query are one copied string.adapters.buildRequestWithLabelValuescalls. The size histograms are still resolved on first use, so a combination that never carried a body still has norequest_size_bytesseries.WithLabelValuesruns outside the set's lock because it panics on invalid UTF-8.ownStringscopies the request strings the span and metric attributes use into one allocation.normalizeMethodreturns its own constants. String and string-slice values fromCustomAttributesandCustomMetricAttributesare copied as they are appended (keys are not).FastHandlerand itsWithGrouphandler, which format before they return) gets copies of every string value, including values inside groups.FastHandleris unchanged.FastHandler.The request body is not copied. net/http forbids reading
r.BodyafterServeHTTPreturns.One difference from net/http remains: with the headers now present, the adapted request on epoll and io_uring also carries
Hostinr.Header, asadapters.WrapMiddleware's already did. net/http's own server moves it tor.Hostonly.r.Hostis set in both cases.Tests
Each test has five arms: std, and epoll and io_uring with sync and async (
AsyncHandlers) handlers. Requests with the same layout and different values go over one keep-alive connection, and what the API kept from an earlier request must still read that request.TestAdaptRequestCarriesHeaders(#720), rootTestAdaptKeptRequestStringsSurviveNextRequest, rootr.URL.Path=/a/cccc,r.Method=MKCOL)TestWrapMiddlewareKeptStringsSurviveNextRequest, middleware/adaptersTestLabelValuesSurviveNextRequest, middleware/metricsGathererrors)TestLabelValuesSurviveConnectionReuse, middleware/metricsion: SECRETfrom another connection; 42 and 36Gathererrors). PASS on the two async arms, see belowTestAttributesSurviveNextRequest, middleware/otelurl.path=/o/cccc,http.request.method=MKCOL)TestKeptRecordSurvivesNextRequest, middleware/logger"Tests on main" is the test commit alone, run on a842109 and again after rebasing onto 2776d4c (#696 landed meanwhile), with the same results. std passes every test at every commit. On main, 26 of the 28 native arms fail. The two that pass are the cross-connection test's async arms: with async handlers the request is parsed from the dispatch double buffer (
asyncInBuf), and in this layout no series read the other connection's bytes in 20 rounds. They stay as coverage. The same-connection metrics test covers async handlers.Runs used Docker linux/arm64 in the CI shape (golang:1.27,
--cpus 4,seccomp=unconfined, memlock 8 MiB, so io_uring gets one worker),-race, one container per package, andCELERIS_REQUIRE_IOURING_WORKERS=1, so no io_uring arm could be dropped. Counts are tallied from--- PASS/FAIL/SKIP:lines only.Mutants. There were 31 mutants, each re-introducing one view or dropping the materialize call: 6 in Adapt, 5 in
WrapMiddleware, 3 in metrics (each routes one metric around the set with the request's strings), 13 in otel (one per copied field,normalizeMethod, and the custom-attribute copies), and 4 in logger. All 31 were killed. A mutant counted as killed only if its test failed on all four native arms while std passed. Each mutant was restored withcp, and the restore was proved by sha256. One mutant did not compile on its first version (munused); it was rewritten, killed, and the invalid log kept. The mutants and the benchmarks below ran before the rebase onto 2776d4c. The rebase changed no commit's patch (git range-diff), and #696 touches only the io_uring driver-conn path, none of these files or the HTTP request path. The mutants' head differs from this one only in sixdefer conn.Close()lines of the tests, rewritten for errcheck.Whole-package suites (
-race -v) at head (root rerun after the rebase): root 368 passed, 0 failed (the 1 top-level and 4 nested SKIPs are the pre-existing adaptive retime tests); middleware/adapters 23, logger 116, metrics 47, otel 63 passed, 0 failed, 0 skipped. golangci-lint v2.13 (GOOS=linux) reports 0 issues on the changed packages, and actionlint passes.CI. The package steps already run these packages, but without
-v, and each test drops its io_uring arms silently when the probe finds no ring. A new Unit step runs the seven tests by name withCELERIS_REQUIRE_IOURING_WORKERS=1and an exact tally: 7 top-level tests, 35 arms (14 io_uring), no SKIP line, and everygo testmust exit 0.Cost (per request)
Benchmarks run each site through a test Context (
celeristest, no engine), with a typical API request's headers. Base isa842109and head is this branch, built as test binaries and run interleaved A B A B for 8 rounds in one container (golang:1.27 linux/arm64,--cpus 4) under the laptop's exclusive timing lock. B/op and allocs/op are exact. ns/op comes from a shared laptop. Δ ns is the median of the 8 per-round (head − base) differences.Adaptadapters.WrapMiddleware(pass-through)LabelFuncslabelFastHandlerslog.JSONHandlerA trivial handler's per-request time depends on what is counted:
WrapMiddleware+58%, otel with no-op providers +22%, and logger with a handler other thanFastHandler+64%. A new metrics label combination is +35%, paid once per combination.slog.JSONHandlerat +0.6%.Each copying site adds one allocation per request (plus one per custom otel string attribute). metrics now costs less than before on every request whose label combination was seen before.
FastHandlerand the OTel SDK path show no change beyond the noise. A cheaper logger option would skip the copies forslog.JSONHandler/slog.TextHandler, which format before they return. It is left out because theirReplaceAttrcallback can keep a value;FastHandlerusers pay nothing.Overlap
middleware/otel/carrier.goorcontext_response.go. Inmiddleware/otelit changes onlyotel.goandconfig.go, and addsretained_attrs_linux_test.go; fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 changescarrier.goand addsdetached_context_alias_linux_test.go. No file in the otel package is touched by both PRs. The only file touched by both is.github/workflows/ci.yml: this PR inserts its step after themiddleware/otelstep (hunk@@ -413,6 +413,45 @@), and fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 inserts its step after the websocket step (@@ -466,6 +466,47 @@).git merge-treeof this branch with fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723's head (c98f068) merges cleanly, and the merged tree passesgo vet(GOOS=linux) for root, adapters, logger, websocket, sse, otel and metrics. The test helpers here are namedkept*, and fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723's arec714*.git merge-treeis also clean against fix(websocket): never strand a chunk that spills after the handler drained the channel (celeris#705); fail the start helpers fast with Start's error (celeris#706) #730, fix(iouring): run every driver op through the engine's own duplicate of the socket, and count every cancel until its CQE, so closing after UnregisterConn is safe (celeris#691, celeris#707) #696 and test(iouring): judge the SEND_ZC first-completion window under -race, with its detachMu mutant (celeris#587) #693 (which also add CI steps) and against fix(server): shut down on every context cancel, and return only after it (celeris#673) #692, fix(epoll): never park the loop on a running async handler, and leave the live set to the loop on an async hijack (celeris#669, celeris#668) #698 and test: run the epoll sendfile e2e tests, and let CI forbid the celeris#592 rig's io_uring skips (celeris#684) #702.Context.Hijackpools the buffer while the hijacker holds views) and middleware/session: with WriteBehind on epoll and io_uring, a loaded session is saved under the NEXT request's session ID (the queued id is a view of the receive buffer): cross-session write #731 (session write-behind) are separate lanes.