refactor: streamline request handler selection and context management for improved performance#4233
refactor: streamline request handler selection and context management for improved performance#4233ReneWerner87 wants to merge 3 commits intomainfrom
Conversation
… for improved performance
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRequest-handler selection is centralized into a Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as fasthttp.Server
participant App
participant selectReqHandler as selectRequestHandler()
participant defaultHandler as defaultRequestHandler
participant ctxPool as Context Pool
participant router as app.next(ctx)
Client->>Server: HTTP Request
Server->>App: Handler(reqCtx)
alt hasCustomCtx is true
App->>selectReqHandler: customRequestHandler path
selectReqHandler-->>App: customRequestHandler
else hasCustomCtx is false
App->>selectReqHandler: defaultRequestHandler path
selectReqHandler-->>App: defaultRequestHandler
end
App->>defaultHandler: Execute selected handler
defaultHandler->>ctxPool: acquireDefaultCtx()
ctxPool-->>defaultHandler: *DefaultCtx (reset & ready)
defaultHandler->>defaultHandler: Clear flash messages<br/>(raw header scan)
defaultHandler->>router: app.next(ctx)<br/>(route matching)
router-->>defaultHandler: Response prepared
alt ctx.abandoned == false
defaultHandler->>ctxPool: Release ctx to pool
end
defaultHandler-->>Server: Response
Server-->>Client: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4233 +/- ##
==========================================
- Coverage 91.17% 91.04% -0.14%
==========================================
Files 123 123
Lines 12084 12121 +37
==========================================
+ Hits 11018 11035 +17
- Misses 668 682 +14
- Partials 398 404 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request optimizes the request handling path by introducing specialized handlers (defaultRequestHandler and customRequestHandler) to avoid interface assertions and unnecessary defer calls. It refactors context management, including a more efficient way to acquire and release default contexts, and optimizes path handling and flash cookie detection. Additionally, it updates documentation and removes redundant release methods from the request and response interfaces. I have no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR refactors Fiber’s request handling path to reduce per-request overhead, primarily by splitting default vs custom context request handlers and tightening hot-path operations in routing and context management.
Changes:
- Introduces separate
defaultRequestHandlerandcustomRequestHandler, and selects between them viaselectRequestHandler(). - Reduces allocations/overhead in routing and path handling (precomputes unsafe string conversions; replaces lowercasing helper with a local fast-path).
- Adjusts context lifecycle cleanup (tracks whether a user context was stored; simplifies req/res release mechanics; updates related interface docs).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| router.go | Precomputes route match strings; splits request handlers; inlines some hot-path checks. |
| app.go | Routes fasthttp handler setup through selectRequestHandler() for default/custom ctx. |
| ctx_interface.go | Adds acquireDefaultCtx to avoid interface/type assertion overhead in default path. |
| ctx.go | Optimizes detection-path lowercasing; refines user-context storage/cleanup; removes req/res release calls. |
| errors_internal.go | Adds a dedicated panic error for default ctx pool type assertion failures. |
| req_interface_gen.go | Doc clarifications around multipart parsing respecting BodyLimit; removes release() from interface. |
| req.go | Removes DefaultReq.release() implementation. |
| res_interface_gen.go | Doc tweaks; clarifies getLocationFromRoute semantics; removes release() from interface. |
| res.go | Removes DefaultRes.release() implementation. |
| ctx_interface_gen.go | Mirrors doc clarifications for multipart + AutoFormat and getLocationFromRoute. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ctx.go (1)
669-678: Consider expressingappendLowerBytesviautils/v2.
utils.ToLowerBytesalready provides a lookup-table-based ASCII lower-case ingithub.com/gofiber/utils/v2. The current single call site can be written as:♻️ Optional refactor
- if !c.app.config.CaseSensitive { - c.detectionPath = appendLowerBytes(c.detectionPath, c.path) - } else { - c.detectionPath = append(c.detectionPath[:0], c.path...) - } + c.detectionPath = append(c.detectionPath[:0], c.path...) + if !c.app.config.CaseSensitive { + c.detectionPath = utils.ToLowerBytes(c.detectionPath) + }…and
appendLowerBytescan be removed.This trades the current single-pass copy+lower for a copy followed by an in-place lookup-table lower. Benchmarks should validate which is faster on the targeted hot path; if the single-pass branch wins, ignore. Otherwise, leaning on the shared helper avoids hand-rolling a primitive.
As per coding guidelines: "Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ctx.go` around lines 669 - 678, The function appendLowerBytes implements ASCII lowercasing manually; replace its single call site to use utils.ToLowerBytes from github.com/gofiber/utils/v2 and remove appendLowerBytes; update imports to add utils (or reuse existing utils import), ensure the call site expects the same behavior (in-place/lowered copy) and run benchmarks if you want to validate performance before deleting appendLowerBytes.router.go (1)
442-446: Consolidate flash-cookie checks by usinghasFlashCookiehelper from both handlers.
defaultRequestHandlerduplicates the flash-cookie optimization inline (raw-header scan followed byCookie(FlashCookieName) != nil), whilecustomRequestHandlercalls the existinghasFlashCookiehelper that performs the identical checks. SincehasFlashCookiealready encodes the optimized fast path (lines 50–61 in redirect.go), replace the inline check indefaultRequestHandlerwith a call tohasFlashCookiefor consistency. This prevents future flash-cookie semantics changes from requiring updates at both call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router.go` around lines 442 - 446, The inline raw-header + Cookie(FlashCookieName) check in defaultRequestHandler duplicates logic already implemented in hasFlashCookie; replace that conditional with a call to hasFlashCookie so defaultRequestHandler uses the same optimized fast-path helper, and then call ctx.Redirect().parseAndClearFlashMessages() as before (i.e., remove the rawHeaders/bytes.Contains branch and invoke hasFlashCookie to decide whether to parse and clear flash messages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ctx_interface.go`:
- Around line 84-93: acquireDefaultCtx currently panics if app.pool.Get() isn't
a *DefaultCtx, which can occur during a race if setCtxFunc is called after the
server starts (app.newCtxFunc non-nil → pool.New returns CustomCtx) while
app.server.Handler still points to defaultRequestHandler; to fix, either assert
in setCtxFunc that it must be called before Listen() (e.g., check app.server ==
nil and return or panic with a clear message) or make acquireDefaultCtx
resilient by detecting a non-*DefaultCtx from app.pool.Get() and delegating to
the generic AcquireCtx / customRequestHandler path (release the non-matching
object if needed and obtain a proper ctx via AcquireCtx) so in-flight requests
never panic—update setCtxFunc, acquireDefaultCtx, and any pool.New logic to
implement one of these two approaches.
In `@router.go`:
- Around line 428-463: Add a single deferred cleanup at the start of
defaultRequestHandler to match customRequestHandler (e.g., defer
app.ReleaseCtx(ctx)) so the ctx is always returned to the pool even if
app.next(ctx) or SendStatus panics, then remove the three duplicated blocks that
manually check ctx.abandoned.Load() and call ctx.release(); app.pool.Put(ctx)
(and the early returns that only exist to release the ctx), leaving logic for
methodInt == -1 and error handling intact but relying on ReleaseCtx to do the
actual release and abandonment check.
---
Nitpick comments:
In `@ctx.go`:
- Around line 669-678: The function appendLowerBytes implements ASCII
lowercasing manually; replace its single call site to use utils.ToLowerBytes
from github.com/gofiber/utils/v2 and remove appendLowerBytes; update imports to
add utils (or reuse existing utils import), ensure the call site expects the
same behavior (in-place/lowered copy) and run benchmarks if you want to validate
performance before deleting appendLowerBytes.
In `@router.go`:
- Around line 442-446: The inline raw-header + Cookie(FlashCookieName) check in
defaultRequestHandler duplicates logic already implemented in hasFlashCookie;
replace that conditional with a call to hasFlashCookie so defaultRequestHandler
uses the same optimized fast-path helper, and then call
ctx.Redirect().parseAndClearFlashMessages() as before (i.e., remove the
rawHeaders/bytes.Contains branch and invoke hasFlashCookie to decide whether to
parse and clear flash messages).
🪄 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
Run ID: cf125ae5-1365-42d9-b24a-bb88f6e37a51
📒 Files selected for processing (10)
app.goctx.goctx_interface.goctx_interface_gen.goerrors_internal.goreq.goreq_interface_gen.gores.gores_interface_gen.gorouter.go
💤 Files with no reviewable changes (2)
- req.go
- res.go
Optimizes Fiber’s default request hot path while preserving custom context behavior and panic-safe context cleanup.
Changes:
*DefaultCtxpath avoids unnecessary interface/type dispatchhasFlashCookieviewBindMapBenchmarks:
Benchmark_Communication_Flow:39.88 ns/op->35.71 ns/opBenchmark_Router_Next_Default:33.23 ns/op->30.23 ns/opBenchmark_Router_Handler:77.84 ns/op->75.97 ns/opBenchmark_Router_Chain: unchanged in final review-safe versionNo allocation regressions.