Repository navigation
fix(router): grow PathValues for contexts created before a route with more params - #3157
Merged
Merged
Conversation
… more params 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, with both DefaultRouter and ConcurrentRouter. Pooled Contexts keep the PathValues capacity they were created with. DefaultRouter.Route already detected the short capacity, but allocated a zero-length slice and then set values by index. Grow the Context's PathValues at full length instead. The Context keeps the new slice, so later requests through it do not grow it again. Document in the Router contract that Route must handle such Contexts. Refs #1705
Address review nits: the grow branch also covers Contexts created without an Echo and routes added through Router directly, the Router contract explains why routes should be added through Echo, and the regression test sits with the other PathValues tests.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix correctly addresses the panic and is covered by deterministic and integration-level regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes stale pooled contexts panicking after routes with additional path parameters are registered.
Changes:
- Grows
PathValuesto an indexable length when capacity is insufficient. - Documents the router capacity contract.
- Adds deterministic and end-to-end regression tests.
| File | Description |
|---|---|
router.go |
Fixes PathValues growth and updates the router contract. |
router_test.go |
Tests stale contexts across route outcomes. |
echo_test.go |
Tests route addition after serving requests. |
💡 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 more path params after Echo has served requests panics with
index out of range [0] with length 0on the next request that matches it. This happens with bothDefaultRouterandConcurrentRouter, even thoughNewConcurrentRoutersays routes can be added after the server has started. The faulty line is in every v5 release (v5.0.0 through v5.4.0).Repro: register
GET /static, serve/static, registerGET /users/:id, serve/users/42.Cause: pooled
Contexts keep thePathValuescapacity they were created with.DefaultRouter.Routealready detected the short capacity, but it allocated a zero-length slice (make(PathValues, 0, max)) and then set values by index. The same branch is reached by Contexts created without an Echo and when routes are added throughRouter().Adddirectly; both panic on master too.Fix: grow the Context's
PathValuesat full length in that existing branch. The Context keeps the new slice, so it is grown once, whatever route the request matches (static, param, 404 or 405), until a route with even more params is added. TheRoutercontract comment now says thatRoutemust handle a Context with a smaller capacity, and that adding routes through Echo is what keeps Contexts sized up front.Cost: the change only touches the branch that already handled short capacity, so steady state is unchanged:
ServeHTTP_*,Echo*APIandRouter*APIbenchmarks: 20 interleaved rounds against master show no regression, and allocations per operation are unchanged.Tests:
TestDefaultRouter_RouteWithContextCreatedBeforeParamRouteAddedis deterministic. It covers param/any, static and not-found cases on a Context created before the routes were added, and checks values and kept capacity. On master the param case panics, and the static and not-found cases fail the capacity check.TestEcho_AddParamRouteAfterServingcovers the end-to-end repro on the default and concurrent routers. It depends onsync.Poolreturning the same Context, which-racesometimes does not.ConcurrentRouterwas clean; it is not committed.Refs #1705 (the same root cause, reported in 2020).