🧹 chore: Improve Performance of Fiber Router #3261
Conversation
WalkthroughThis pull request encompasses multiple changes across different files in the Fiber framework. The modifications include updating the Changes
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3261 +/- ##
==========================================
- Coverage 84.42% 84.23% -0.19%
==========================================
Files 116 116
Lines 11497 11519 +22
==========================================
- Hits 9706 9703 -3
- Misses 1374 1391 +17
- Partials 417 425 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: 14a8635 | Previous: 775e0a7 | Ratio |
|---|---|---|---|
| `Benchmark_RoutePatternMatch//api/:param/fixedEnd_ | not_match | _/api/abc/def/fixedEnd - allocs/op` | 14 allocs/op |
BenchmarkUnmarshalitem-4_middleware_limiter |
40.61 ns/op 640.26 MB/s 0 B/op 0 allocs/op |
26.65 ns/op 975.75 MB/s 0 B/op 0 allocs/op |
1.52 |
BenchmarkUnmarshalitem-4_middleware_limiter - ns/op |
40.61 ns/op |
26.65 ns/op |
1.52 |
This comment was automatically generated by workflow using github-action-benchmark.
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app.go (2)
619-622: Tests recommended for dynamicNewCtxFuncchanges
By settingapp.server.Handlertoapp.customRequestHandleron non-nilnewCtxFunc, the server’s behavior changes significantly. Consider adding tests that confirm correct fallback todefaultRequestHandlerwhennewCtxFuncis nil.
875-879: Handler returns custom or default
This block selects betweenapp.customRequestHandlerandapp.defaultRequestHandler. Looks functionally correct. Adding minimal test coverage ensures stable switching whenevernewCtxFuncis set or unset.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 877-878: app.go#L877-L878
Added lines #L877 - L878 were not covered by testsctx_interface_gen.go (1)
353-355: New Drop() method
TheDrop()method can be valuable for quietly terminating connections (like DDoS mitigation). Ensure calling it doesn’t break existing middlewares that assume a response is always sent.Could you document how or when this should be invoked in high-load cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/linter.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
Makefile(1 hunks)app.go(3 hunks)binder/mapping.go(1 hunks)ctx_interface_gen.go(1 hunks)path.go(1 hunks)router.go(4 hunks)router_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- binder/mapping.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app.go
[warning] 877-878: app.go#L877-L878
Added lines #L877 - L878 were not covered by tests
[warning] 1069-1069: app.go#L1069
Added line #L1069 was not covered by tests
router.go
[warning] 218-218: router.go#L218
Added line #L218 was not covered by tests
[warning] 232-233: router.go#L232-L233
Added lines #L232 - L233 were not covered by tests
[warning] 249-249: router.go#L249
Added line #L249 was not covered by tests
[warning] 270-270: router.go#L270
Added line #L270 was not covered by tests
🔇 Additional comments (8)
router.go (4)
214-243: Add test coverage for edge conditions
Lines #218 (panic) and #232–233 (flash cookie handling) appear uncovered by tests. Consider adding unit or integration tests to validate the behavior when the context pool fails to provide a *DefaultCtx and when request headers include flash cookies.
Do you want help generating a test function for these lines?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-218: router.go#L218
Added line #L218 was not covered by tests
[warning] 232-233: router.go#L232-L233
Added lines #L232 - L233 were not covered by tests
245-270: Extend coverage for custom request handling
Similar to the default handler, lines #249 (panic) and #270 (internal server error state) lack explicit test coverage. Ensure that custom context scenarios are tested to guarantee stability and maintain confidence in your customRequestHandler.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 249-249: router.go#L249
Added line #L249 was not covered by tests
[warning] 270-270: router.go#L270
Added line #L270 was not covered by tests
327-376: Registration logic looks good
The path normalization and route registration improvements appear consistent. The single-pass path cleaning and constraints for empty handlers (panics) make maintenance easier. Great work!
68-106: Consider verifying edge cases in route matching
This logic correctly checks for root (r.root) and r.star. However, please ensure that complex edge cases (e.g., detectionPath unexpectedly empty or partial prefix matches) are handled as intended, especially when r.use is true.
Run this script to search relevant tests covering Route.match logic:
✅ Verification successful
Let me check the test implementations to verify the edge cases.
Route matching logic is well-tested for edge cases
The test suite comprehensively covers all edge cases in the route matching logic:
- Empty paths and root handling (
Test_Route_Match_Root) - Wildcard/star patterns (
Test_Route_Match_Star) - Middleware prefix matches (
Test_Route_Match_Middleware_HasPrefix) - Parameter parsing (
Test_Route_Match_Parser) - Special character handling (
Test_Route_Match_UnescapedPath,Test_Route_Match_WithEscapeChar) - Root middleware behavior (
Test_Route_Match_Middleware_Root)
The implementation correctly handles all these cases, including empty paths, partial matches, and special characters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script looks for test methods covering `Route.match` usage
rg -A 8 "func Test_Route_Match"
Length of output: 2847
Script:
#!/bin/bash
# Get the full test implementations
rg -B 2 -A 25 "^func Test_Route_Match" router_test.go
Length of output: 6582
app.go (1)
1068-1072: Initialize the correct request handler
This chunk replicates the logic above, bridging the custom or default handler based on newCtxFunc. Ensuring consistency between Handler() and init() is key. Good design.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1069-1069: app.go#L1069
Added line #L1069 was not covered by tests
router_test.go (1)
594-615: Parallel benchmark is a great addition
The new function Benchmark_Router_Next_Default_Parallel significantly broadens performance testing under concurrency. Consider adding testcases for heavier routes or dynamic handlers to measure real-world usage.
path.go (1)
623-632: Impressive single-pass removal
Shifting to a manual byte-slice approach for escaping yields meaningful performance gains. The logic is straightforward, and skipping copy operations should reduce allocations.
Makefile (1)
38-38: LGTM! Verify the golangci-lint version adoption.
The minor version bump of golangci-lint is a good maintenance practice. Let's verify the version's stability and adoption rate.
✅ Verification successful
Golangci-lint version update is safe to proceed
The verification shows:
- The version v1.62.2 is only referenced in the Makefile, indicating this is a new adoption
- The GitHub workflow uses the golangci-lint GitHub action v6, which is version-agnostic and compatible with the CLI version being used
- No specific version constraints or issues were found in the documentation
Since this is a patch version update (v1.62.0 → v1.62.2) and the tooling setup is properly maintained through both Makefile and GitHub Actions, this change is safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the adoption rate and stability of golangci-lint v1.62.2
# Check if this version is referenced in go.mod files across the codebase
rg -l "golangci-lint.*v1.62.2"
# Look for any version-specific comments or issues mentioned in docs
fd -t f -e md | xargs rg "golangci-lint.*1\.62\.2"
# Check if there are any version constraints in GitHub workflows
fd -t f -g "*.yml" -g "*.yaml" .github/workflows/ | xargs rg "golangci-lint.*@v"
Length of output: 306
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router.go (2)
214-243: Well-structured handler split with room for optimizationThe separation into
defaultRequestHandlerandcustomRequestHandlerimproves type safety and performance. Consider extracting common logic (flash message handling, error handling) into helper functions to reduce duplication.+ func (app *App) handleFlashMessages(req *fasthttp.Request) bool { + rawHeaders := req.Header.RawHeaders() + return len(rawHeaders) > 0 && bytes.Contains(rawHeaders, []byte(FlashCookieName)) + } + + func (app *App) handleError(ctx any, err error) { + if catch := ctx.(interface{ App() *App }).App().ErrorHandler(ctx, err); catch != nil { + _ = ctx.(interface{ SendStatus(int) error }).SendStatus(StatusInternalServerError) + } + }Also applies to: 245-273
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-218: router.go#L218
Added line #L218 was not covered by tests
[warning] 232-232: router.go#L232
Added line #L232 was not covered by tests
343-376: Consider validating HTTP method earlierThe route registration logic is solid, but method validation could be moved before the expensive path parsing operations to fail fast on invalid methods.
- parsedRaw := parseRoute(pathRaw, app.customConstraints...) - parsedPretty := parseRoute(pathPretty, app.customConstraints...) - for _, method := range methods { method = utils.ToUpper(method) if method != methodUse && app.methodInt(method) == -1 { panic(fmt.Sprintf("add: invalid http method %s\n", method)) } + + parsedRaw := parseRoute(pathRaw, app.customConstraints...) + parsedPretty := parseRoute(pathPretty, app.customConstraints...)app_test.go (1)
617-642: Refactor to unify tests for custom methods
The test logic inTest_App_All_Method_Testmirrors the approach inTest_App_Add_Method_Test. While this is fully functional, consider consolidating or parameterizing these tests to reduce duplication and streamline maintenance of custom method tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app_test.go(1 hunks)router.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
router.go
[warning] 218-218: router.go#L218
Added line #L218 was not covered by tests
[warning] 232-232: router.go#L232
Added line #L232 was not covered by tests
[warning] 249-249: router.go#L249
Added line #L249 was not covered by tests
[warning] 256-258: router.go#L256-L258
Added lines #L256 - L258 were not covered by tests
[warning] 263-264: router.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 269-270: router.go#L269-L270
Added lines #L269 - L270 were not covered by tests
🔇 Additional comments (5)
router.go (4)
68-106: LGTM! Route matching optimizations look solid
The optimizations to the route matching logic are well-implemented:
- More efficient root path detection using length check
- Clear separation of wildcard, parameter, and middleware matching logic
- Improved middleware route handling with explicit root path cases
218-218: Add test coverage for error paths
Several critical code paths lack test coverage:
- Type assertion failures
- Invalid HTTP method handling
- Flash message processing
- Error handling scenarios
Please add test cases to cover these scenarios to ensure reliability.
Would you like me to help generate comprehensive test cases for these paths?
Also applies to: 232-232, 249-249, 256-258, 263-264, 269-270
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-218: router.go#L218
Added line #L218 was not covered by tests
327-342: LGTM! Efficient path normalization implementation
The path normalization logic is well-structured and efficient:
- Precomputes normalization once
- Handles all edge cases (empty paths, leading/trailing slashes)
- Respects configuration settings (case sensitivity, strict routing)
255-255: Verify methodInt implementation as mentioned
As noted in the previous review, there might be a bug with app.methodInt(). Please verify the implementation and consider adding a check.
app_test.go (1)
609-616: Add clarity on custom method naming
These lines introduce a new custom method "JANE" for testing. The logic correctly verifies the method’s handling. If you intend to keep "JANE" for broader usage, consider documenting in the Fiber or project-specific docs how custom methods are added and recognized, so that it's easy for developers to discover and maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app_test.go (2)
584-608: LGTM! Well-structured test for custom HTTP method handling.The test comprehensively covers:
- Adding custom method "JOHN"
- Successful request handling (200 OK)
- Method not allowed case (405)
- Not implemented case (501)
- Panic behavior for invalid method addition
Consider adding test cases for:
- Multiple custom methods in a single Add() call
- Case sensitivity validation
- Special characters in method names
Line range hint
610-635: LGTM! Good coverage of All() method with custom HTTP methods.The test effectively validates:
- All() method with custom method "JOHN"
- Adding new method "JANE" after All()
- Status code validation
Consider adding test cases for:
- Concurrent requests with multiple methods
- Error handling for malformed requests
- Header validation in responses
ctx_test.go (1)
130-157: LGTM! Good test coverage for custom context with custom methods.The test effectively validates:
- Custom context creation with custom method
- Request handling with custom method "JOHN"
- Panic behavior for invalid method "JANE"
Consider adding test cases for:
- Context behavior with middleware chain
- Error handling in custom context
- Memory leaks validation with deferred cleanup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app_test.go (1)
610-628: Consider adding test cases for standard HTTP methods.While the test covers custom method handling, it would be beneficial to verify that the
Allroute responds correctly to standard HTTP methods (GET, POST, etc.) as well.Add test cases to verify standard HTTP methods:
app.All("/doe", testEmptyHandler) resp, err := app.Test(httptest.NewRequest("JOHN", "/doe", nil)) require.NoError(t, err, "app.Test(req)") require.Equal(t, StatusOK, resp.StatusCode, "Status code") +// Test standard HTTP methods +for _, method := range []string{MethodGet, MethodPost, MethodPut, MethodDelete} { + resp, err := app.Test(httptest.NewRequest(method, "/doe", nil)) + require.NoError(t, err, "app.Test(req)") + require.Equal(t, StatusOK, resp.StatusCode, fmt.Sprintf("Status code for %s", method)) +} // Add a new method require.Panics(t, func() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app_test.go(1 hunks)
🔇 Additional comments (1)
app_test.go (1)
590-608: LGTM! Well-structured test cases for custom method handling.
The test comprehensively covers:
- Success case with custom method
- Method not allowed case
- Unknown method case
- Panic case when adding method after initialization
Description
requestHandlerinto two methods. One for CustomCtx and one for DefaultCtx.Improvements to RemoveEscapeChar()
After:
Before:
Router Improvements
Benchmark between v2, main, and this PR
Type of change