Repository navigation
fix(router): grow param values for contexts created before a route with more params (v4) - #3158
Merged
Merged
Conversation
…th more params (v4) Adding a route with more path params after Echo has served requests panics with "index out of range" on the next request that matches it. Pooled contexts keep the pvalues length they were created with, and Router.Find sets values by index. Grow the context's pvalues when Find is about to set a value past its length. Static routes do not reach these branches. The context keeps the new slice, so later requests through it do not grow it again. Refs #1705
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix preserves routing state and has appropriate regression coverage for both growth paths.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes stale pooled contexts so routes added later with more path parameters no longer panic.
Changes:
- Dynamically grows context parameter storage during router lookup.
- Preserves captured values during growth.
- Adds router-level and end-to-end regression tests.
| File | Description |
|---|---|
router.go |
Grows parameter storage in param and wildcard branches. |
router_test.go |
Tests stale contexts against late param and wildcard routes. |
echo_test.go |
Tests the reported pooled-context scenario end to end. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding a route with path params after Echo has served requests can panic with
index out of range [0] with length 0. It happens in sequential, single-goroutine use:/files/*) panics in every v4 release: inRouter.Find, or already inContext.Resetfrom v4.1.12 to v4.11.x./users/:id) wrongly returned 404 up to v4.1.11. From v4.1.12 to v4.11.x it panics inContext.Reset, on the next request of any kind.Router.Find, on requests whose routing reaches the late param or any node. That includes some 404s and 405s.Repro: register
GET /static, serve/static, registerGET /users/:id, serve/users/42.Cause: pooled contexts keep the
pvalueslength they were created with (*e.maxParamat the time), andRouter.Findsets values by index. #2611 fixedSetParamNames/SetParamValuesandReset, but notFind.Fix: when
Findis about to set a param or any value past the context'spvalueslength, it grows the slice to*r.echo.maxParam, keeps the values already set, and stores it on the context. Static routes never reach these branches.This does not make it safe to add routes concurrently with serving. That is still unsupported in v4, as discussed in #1705.
Cost: 20 interleaved rounds against v4 show +0.9% geomean on router benchmarks, with 0 allocs. Static routes are unchanged. Param-heavy routes rise up to about +2.8%, which is about 0.5–1.4 ns per param lookup. This was the cheapest of three placements measured (top of
Find: +1.1%;Resetwithclear(): +2.6%).Tests:
TestEchoAddParamRouteAfterServingis the end-to-end repro.TestRouterFindWithContextCreatedBeforeParamRouteAddedcovers a param route and an any route on a context created before the routes were added; each case covers one grow branch.Refs #1705