⚡ perf: adopt gofiber/utils v2.4.0 helpers and optimize adaptor/proxy hot paths#4557
Conversation
Use the helpers introduced in gofiber/utils v2.4.0 to cut allocations and speed up per-request work: - req.go: parseHTTPDate now delegates to utils.ParseHTTPDate, which keeps net/http.ParseTime semantics (IMF-fixdate plus the obsolete RFC 850 and asctime formats) behind an allocation-free fast path, removing the string(date) conversion on the fallback. - req.go: isTrustedProxyIP parses proxy IPs with utils.ParseIPv4 and utils.ParseIPv6 instead of netip.ParseAddr, skipping the error allocation for invalid client-supplied addresses. - redirect.go: Route escapes query values straight into the pooled buffer with utils.AppendQueryEscape instead of allocating an intermediate string via url.QueryEscape. - middleware/cache: the key generator escapes query keys and values in place with utils.AppendQueryEscape, dropping two intermediate strings per parameter pair; the buffer bound check is restated post-append in an exactly equivalent form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
- middleware/cache: format the Date header with utils.AppendHTTPDate, a direct field writer that is ~3.8x faster than fasthttp's time.AppendFormat-based AppendHTTPDate (150ns -> 39ns per call), used on both the cache-hit and cache-store paths. - middleware/cache: parse the Date header with utils.ParseHTTPDate. The fast path matches fasthttp's for canonical IMF-fixdate input, and the fallback now also accepts the obsolete RFC 850 and asctime forms that RFC 9110 §5.6.7 requires recipients to handle. - middleware/paginate: render pagination query strings with a url.Values.Encode equivalent that escapes via utils.AppendQueryEscape into a single buffer instead of allocating an intermediate string per key and value; output is pinned byte-for-byte to Encode by a new test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
net.ParseIP is netip.ParseAddr minus zoned addresses, so screening out '%' first and delegating to utils.ParseIPv4/ParseIPv6 (new in utils v2.4.0) keeps acceptance identical while avoiding the net.IP slice allocation on success and the error allocation on failure: - middleware/hostauthorization: IPv6-literal Host values are validated allocation-free on the per-request syntax check. - client: the cookie jar's isIPLiteral does the same for cookie domain matching. Both behaviors are pinned by new tests, including zoned-address rejection and IPv4-mapped IPv6 acceptance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
An adversarial audit of the earlier utils.ParseHTTPDate conversions surfaced two fidelity gaps: - middleware/cache parsed the Date header with utils.ParseHTTPDate (IMF-fixdate plus the obsolete RFC 850 and asctime forms) while Expires still went through fasthttp.ParseHTTPDate (IMF-fixdate only), so a response dating itself consistently in RFC 850 got its Date honored but its Expires routed to the parse-error force-revalidate path. Expires now uses utils.ParseHTTPDate too; a new test pins that a future RFC 850 Expires enables caching. - utils.ParseHTTPDate tolerates surrounding ASCII whitespace, which net/http.ParseTime does not — reachable via handler-set dates carrying stray whitespace (fasthttp rewrites embedded newlines to spaces). This tolerance matches RFC 9110 §5.5's exclusion of leading/trailing OWS from field values, so it is kept, documented on parseHTTPDate, and pinned by a Fresh() test case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
adaptor (net/http -> fiber path in handlerFunc): - Build the fasthttp request directly into the pooled RequestCtx instead of filling a standalone Request and CopyTo-ing it across. Init2 only touches connection metadata and buffer-retention flags, so in-place construction is exactly fasthttp's own Init order minus the full header+body copy. A fresh-body 1MB request drops ~35% in ns/op and four allocations. - Pool the noopConn together with the RequestCtx and share one stateless disabledLogger, removing two per-request allocations. - resolveRemoteAddr takes a fast path for numeric ip:port literals — the only form net/http servers produce — via utils.ParseIPv4/ParseIPv6 and a strict decimal port parse, skipping net.ResolveTCPAddr's resolver machinery; hostnames and service ports still fall back to it. A new test pins the fast path to ResolveTCPAddr's results, including IPv6 zones and leading-zero forms. proxy: - Strip Connection-listed headers by iterating the header value bytes in place instead of materializing a string copy and a name slice: BenchmarkStripHopByHop_WithConnection goes from 1565ns/72B/3allocs to ~958ns with zero allocations. The names alias the value buffers, which is sound because fasthttp's Del re-slices the entry list without rewriting other entries' buffers. The fuzz harness now exercises deletion against a real header. - followRedirects renders each hop's URL once instead of twice, removing a URL.String() allocation per hop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
The header copy loops in handlerFunc and HTTPMiddleware called Header.Set once per value, so repeated header lines (e.g. two X-Forwarded-For entries) collapsed to the last value. Set the first value — keeping replace semantics for keys already present on the target request — and Add the rest. fasthttp's Add intentionally keeps singleton semantics for Cookie/Content-Type/etc., so those behave as before. New tests pin both conversion paths and fail against the old code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR standardizes IP and HTTP-date parsing, improves adapter pooling and multi-value header preservation, adds literal remote-address handling, replaces query escaping paths, and rewires proxy connection-header deletion with expanded tests. ChangesRuntime and protocol updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPMiddleware
participant handlerFunc
participant resolveRemoteAddr
participant Fiber
Client->>HTTPMiddleware: send net/http request with repeated headers
HTTPMiddleware->>handlerFunc: copy multi-value headers
handlerFunc->>resolveRemoteAddr: resolve literal or fallback address
handlerFunc->>Fiber: initialize pooled fasthttp context
Fiber-->>Client: return converted response
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4557 +/- ##
==========================================
+ Coverage 93.25% 93.28% +0.03%
==========================================
Files 140 140
Lines 14645 14685 +40
==========================================
+ Hits 13657 13699 +42
+ Misses 615 614 -1
+ Partials 373 372 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/adaptor/adaptor.go`:
- Around line 57-64: Fix the lint failures in pooledCtx and emptyStringTest:
reorder pooledCtx fields according to fieldalignment’s recommended layout, then
update the empty-string assertion in emptyStringTest to use the repository’s
lint-approved empty-string check. Keep behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 24378246-6c90-4370-8a03-c27d8e06ccc8
📒 Files selected for processing (18)
client/cookiejar.goclient/cookiejar_test.goctx_test.gomiddleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.gomiddleware/cache/cache.gomiddleware/cache/cache_test.gomiddleware/cache/keygen.gomiddleware/cache/utils.gomiddleware/hostauthorization/hostauthorization.gomiddleware/hostauthorization/hostauthorization_test.gomiddleware/paginate/page_info.gomiddleware/paginate/paginate_test.gomiddleware/proxy/fuzz_test.gomiddleware/proxy/proxy.gomiddleware/proxy/security.goredirect.goreq.go
- parseDecimalPort: use s == "" (gocritic emptyStringTest). - pooledCtx: conn field first to minimize the trailing pointer span (govet fieldalignment). - paginate test: rename a local that started shadowing the net/url import added for the encodeQueryValues test (gocritic importShadow). - Cover the previously untested branches codecov flagged: empty and oversized and non-numeric port strings in the resolveRemoteAddr equivalence test, and empty header value slices in both header copy loops. Verified with golangci-lint v2.12.2 (the CI version): 0 issues. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/adaptor/adaptor_test.go (1)
1896-1914: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the empty header slice is skipped.
The test sets
X-Emptytonilbut never checks it. A regression that copies an empty header into Fiber would still pass; capturePeekAll("X-Empty")and assert it is empty.Suggested assertion
var got []string +var emptyHeaderValues int app.Get("/", func(c fiber.Ctx) error { for _, v := range c.Request().Header.PeekAll("X-Multi") { got = append(got, string(v)) } + emptyHeaderValues = len(c.Request().Header.PeekAll("X-Empty")) return c.SendStatus(fiber.StatusOK) }) require.Equal(t, []string{"one", "two"}, got) +require.Zero(t, emptyHeaderValues)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/adaptor/adaptor_test.go` around lines 1896 - 1914, Update the test handler in the existing middleware test to capture c.Request().Header.PeekAll("X-Empty") and assert that the captured values are empty, alongside the existing X-Multi assertion. Keep the current X-Multi verification unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@middleware/adaptor/adaptor_test.go`:
- Around line 1896-1914: Update the test handler in the existing middleware test
to capture c.Request().Header.PeekAll("X-Empty") and assert that the captured
values are empty, alongside the existing X-Multi assertion. Keep the current
X-Multi verification unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b1e98d3-7291-4148-b078-b60ab1f80e76
📒 Files selected for processing (3)
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.gomiddleware/paginate/paginate_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- middleware/paginate/paginate_test.go
- middleware/adaptor/adaptor.go
Follow-up to a review note: the HTTPMiddleware multi-value test set an
empty X-Empty value slice to cover the skip branch but never verified
the header stays absent on the fiber request. Capture PeekAll("X-Empty")
and assert it is empty.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 979e23e | Previous: 6639aef | Ratio |
|---|---|---|---|
BenchmarkDecoderedirectionMsg (github.com/gofiber/fiber/v3) |
175.1 ns/op 182.76 MB/s 0 B/op 0 allocs/op |
79.62 ns/op 401.88 MB/s 0 B/op 0 allocs/op |
2.20 |
BenchmarkDecoderedirectionMsg (github.com/gofiber/fiber/v3) - ns/op |
175.1 ns/op |
79.62 ns/op |
2.20 |
This comment was automatically generated by workflow using github-action-benchmark.
|
The benchmark alert on Interleaved A/B on one machine (
I've queued a re-run of the failed benchmark job. Generated by Claude Code |
|
Benchmark check update: the first re-run cleared the Two runs flagging two different benchmarks in untouched code, both flat in controlled A/B, points to runner variance against the single-run baseline rather than a regression. I've queued another re-run; if it trips a third unrelated benchmark, I'll leave it for a maintainer's judgment with this data on record. Generated by Claude Code |
Benchmark_Client_Request_Send_ContextCancel gave the handler a fixed 1ms sleep as the window for the benchmark loop to cancel the request context. On fast CI runners the response wins that race, Send returns a real response, and the nil assertion fails — as seen on the PR benchmark job (AMD EPYC runner). Replace the sleep with an explicit handshake: the handler responds only after the loop has called cancel(), so the context's Done channel is closed strictly before the response can be produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
|
The third benchmark-job failure had a different root cause than the two perf-threshold flakes: Generated by Claude Code |
golangci-lint's misspell rule rejects "cancelled"; rename the handshake channel and comment to "canceled" to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/client_test.go`:
- Line 2405: Update the comment in the benchmark response flow to use the
repository’s “canceled” spelling instead of “cancelled,” without changing the
surrounding behavior or code.
- Around line 2408-2409: Remove the time.Sleep-based ordering in the
cancellation test around cancelledCh. Extend the cancellation handshake so it
acknowledges that the cancellation path has fully drained or released the
request before allowing the response to proceed, then await that acknowledgement
before asserting the response; preserve the existing cancellation behavior and
require.Nil check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5506c5c-9938-4083-960a-836081ee09ba
📒 Files selected for processing (1)
client/client_test.go
Review follow-up: the previous handshake proved cancel() had been called before the handler responded, but the client's select could still in principle see both the Done and response channels ready and pick randomly. core.go's ctx.Done() branch returns immediately and hands the in-flight response to a drainer goroutine, so the loop can receive Send's outcome while the handler is still blocked — at that point the response channel cannot be ready and the cancellation branch is the only possible result. The handler is released only afterwards, and the sleep is gone entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK
Description
Adopts the helper functions introduced in gofiber/utils v2.4.0 (
ParseHTTPDate/AppendHTTPDate,ParseIPv4/ParseIPv6,AppendQueryEscape) across every hot path in the framework where they beat the stdlib/fasthttp equivalent, and follows up with targeted optimizations of the proxy and adaptor middlewares plus two correctness fixes surfaced along the way. Every conversion is either provably behavior-identical or an explicitly documented, test-pinned RFC 9110 compliance improvement.Benchmarks below were taken on the CI-like environment (Intel Xeon @ 2.10 GHz, linux/amd64,
-count=3); absolute numbers are noisy on that VM, the before/after deltas are what matters.Changes introduced
HTTP date parsing/formatting (
req.go,middleware/cache)parseHTTPDate(backsFresh()/If-Modified-Since) now delegates toutils.ParseHTTPDate: net/http.ParseTime semantics (IMF-fixdate + obsolete RFC 850/asctime, RFC 9110 §5.6.7) behind an allocation-free scalar fast path, dropping thestring(date)conversion.Dateheader withutils.AppendHTTPDate(direct field writer) instead offasthttp.AppendHTTPDate(which delegates totime.AppendFormat), on both the cache-hit and cache-store paths.DateandExpireswithutils.ParseHTTPDate, so both share one RFC 9110 acceptance set. Previously a response dating itself consistently in RFC 850 got itsDatehonored while itsExpireswas routed to the parse-error/force-revalidate path. Pinned byTest_CacheExpiresObsoleteFormatAllowsCaching(fails on the old parser).utils.ParseHTTPDatetolerates surrounding ASCII whitespace (RFC 9110 §5.5 OWS exclusion), whichnet/http.ParseTimedoes not; this is documented onparseHTTPDateand pinned by aFresh()test case.Benchmark_Ctx_Fresh_LastModifiedAppendHTTPDate(Date header rendering, micro)IP parsing (
req.go,middleware/hostauthorization,client)isTrustedProxyIPusesutils.ParseIPv4/ParseIPv6instead ofnetip.ParseAddr— identical acceptance, no error allocation for invalid client-supplied addresses.net.ParseIPisnetip.ParseAddrminus zoned addresses (Go ≥1.17), so hostauthorization's IPv6Hostvalidation and the client cookie jar'sisIPLiteralnow screen%and delegate to the utils parsers: identical acceptance, zero allocations on either outcome. Pinned by tests covering zoned rejection, IPv4-mapped IPv6, bracketed literals, and malformed octets.Query escaping (
redirect.go,middleware/cache,middleware/paginate)Redirect.Routeescapes query values straight into the pooled buffer viautils.AppendQueryEscape.url.Values.Encode-equivalent that escapes into a single buffer; output is pinned byte-for-byte againstEncodeacross escaping-heavy inputs.Benchmark_Redirect_Route_WithQueriesBenchmark_defaultKeyGenerator/multiparamAdaptor middleware (
middleware/adaptor)handlerFunc(theFiberHandlerFunc/FiberAppserving path) builds the fasthttp request directly into the pooledRequestCtxinstead of filling a standaloneRequestandCopyTo-ing it across —Init2only touches connection metadata and buffer flags, so in-place construction is fasthttp's ownInitorder minus the full header+body copy.noopConnis pooled together with theRequestCtxand the stateless logger is shared, removing two per-request allocations.resolveRemoteAddrtakes a fast path for numericip:portliterals (the only form net/http servers produce) viautils.ParseIPv4/ParseIPv6+ a strict decimal port parse, skippingnet.ResolveTCPAddr's resolver machinery; hostnames/service ports still fall back. A test pins the fast path toResolveTCPAddr's results (IPv6 zones, leading-zero forms included).handlerFuncandHTTPMiddlewarecalledHeader.Setonce per value, collapsing repeated header lines (e.g. twoX-Forwarded-Forentries) to the last value. NowSetfor the first value +Addfor the rest; fasthttp's singleton headers (Cookie/Content-Type/…) keep their by-design single-value behavior. New tests pin both paths and fail against the old code.FiberHandlerFunc, fresh 1 MB body per requestBenchmark_FiberHandlerFunc/No_ContentProxy middleware (
middleware/proxy)Delre-slices the entry list without rewriting other entries' buffers; the fuzz harness now exercises deletion against a real header to guard exactly that invariant.followRedirectsrenders each hop's URL once instead of twice, removing aURL.String()allocation per redirect hop.BenchmarkStripHopByHop_WithConnectionBenchmarkStripHopByHop_NoConnectionDeliberately not converted (each verified):
url.PathUnescapein extractors/static (stdlib's no-%fast path returns the original string with zero allocations), startup-onlynet.ParseIP/ParseCIDRinapp.go, proxy SSRF checks (interoperate withnet.IPthrough the resolver; dial/DNS dominates),res.goencodeExtValue(RFC 8187 attr-char set differs from any utils helper), and route constraints'time.Parse(user-defined layouts).ExpiresRFC 9110 parsing, adaptor multi-value header preservation.Type of change
Checklist
encodeQueryValues≡url.Values.Encode,resolveRemoteAddr≡ResolveTCPAddr, zoned-IPv6 rejection, multi-value header preservation — the behavioral tests were verified to fail against the pre-change code).🤖 Generated with Claude Code
https://claude.ai/code/session_01E5sm1LHEnvEVDRGsHWFfKK