diff --git a/echo_test.go b/echo_test.go index e7c022541..25da51561 100644 --- a/echo_test.go +++ b/echo_test.go @@ -1966,3 +1966,33 @@ func TestHasDotOrEmptySegment(t *testing.T) { }) } } + +func TestEcho_AddParamRouteAfterServing(t *testing.T) { + var testCases = []struct { + name string + router Router + }{ + {name: "ok, default router", router: NewRouter(RouterConfig{})}, + {name: "ok, concurrent router", router: NewConcurrentRouter(NewRouter(RouterConfig{}))}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := NewWithConfig(Config{Router: tc.router}) + e.GET("/static", func(c *Context) error { return c.String(http.StatusOK, "static") }) + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/static", nil)) + assert.Equal(t, "static", rec.Body.String()) + + // the pooled context from the first request has no PathValues capacity for this route. sync.Pool may hand + // out a new context instead (often under -race); TestDefaultRouter_RouteWithContextCreatedBeforeParamRouteAdded + // covers the stale context deterministically. + e.GET("/users/:id", func(c *Context) error { return c.String(http.StatusOK, c.Param("id")) }) + + rec = httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/users/42", nil)) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "42", rec.Body.String()) + }) + } +} diff --git a/router.go b/router.go index 599c561a5..9d807d3a3 100644 --- a/router.go +++ b/router.go @@ -15,8 +15,11 @@ import ( // Router is interface for routing request contexts to registered routes. // // Contract between Echo/Context instance and the router: -// - all routes must be added through methods on echo.Echo instance. -// Reason: Echo instance uses RouteInfo.Params() length to allocate slice for paths parameters (see `Echo.contextPathParamAllocSize`). +// - all routes should be added through methods on echo.Echo instance. +// Reason: Echo instance uses RouteInfo.Parameters length to allocate slice for paths parameters (see `Echo.contextPathParamAllocSize`), +// so Contexts are created with enough capacity. +// - Router.Route must handle a Context whose PathValues capacity is smaller than the maximum path parameter count +// of its routes (for example, when a route was added after the Context was created, or not through echo.Echo). // - Router must populate Context during Router.Route call with: // - Context.InitializeRoute (IMPORTANT! to reduce allocations use same slice that c.PathValues() returns) // - Optionally can set additional information to Context with Context.Set @@ -916,7 +919,11 @@ var optionsMethodHandler = func(c *Context) error { func (r *DefaultRouter) Route(c *Context) HandlerFunc { pathValues := c.PathValues() if cap(pathValues) < r.maxPathParamsLength { - pathValues = make(PathValues, 0, r.maxPathParamsLength) + // The Context has less capacity than this router's routes need, for example because it was created before a + // route with more params was added. Grow its PathValues at full length so values can be set by index below. The + // Context keeps the new slice, so later requests do not grow it again unless a route with more params is added. + *c.pathValues = make(PathValues, r.maxPathParamsLength) + pathValues = *c.pathValues } else { pathValues = pathValues[0:cap(pathValues)] // resize slice to maximum capacity so we can index set values } diff --git a/router_test.go b/router_test.go index fd7786f95..740569ed9 100644 --- a/router_test.go +++ b/router_test.go @@ -2617,6 +2617,56 @@ func TestRouterParam1466(t *testing.T) { } } +func TestDefaultRouter_RouteWithContextCreatedBeforeParamRouteAdded(t *testing.T) { + var testCases = []struct { + name string + whenURL string + expectRoute string + expectPathValues PathValues + }{ + { + name: "ok, param and any route", + whenURL: "/users/1/files/a.txt", + expectRoute: "/users/:id/files/*", + expectPathValues: PathValues{{Name: "id", Value: "1"}, {Name: "*", Value: "a.txt"}}, + }, + { + name: "ok, static route", + whenURL: "/static", + expectRoute: "/static", + expectPathValues: PathValues{}, + }, + { + name: "ok, route not found", + whenURL: "/missing", + expectRoute: "", + expectPathValues: PathValues{}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + // the context is created while no route has path params, so its PathValues have no capacity + c := e.NewContext(httptest.NewRequest(http.MethodGet, tc.whenURL, nil), httptest.NewRecorder()) + + r := NewRouter(RouterConfig{}) + _, err := r.Add(Route{Method: http.MethodGet, Path: "/users/:id/files/*", Handler: handlerFunc}) + assert.NoError(t, err) + _, err = r.Add(Route{Method: http.MethodGet, Path: "/a/:b/:c/:d", Handler: handlerFunc}) + assert.NoError(t, err) + _, err = r.Add(Route{Method: http.MethodGet, Path: "/static", Handler: handlerFunc}) + assert.NoError(t, err) + + r.Route(c) + + assert.Equal(t, tc.expectRoute, c.Path()) + assert.Equal(t, tc.expectPathValues, c.PathValues()) + // the context keeps the grown capacity, so later requests do not grow it again + assert.Equal(t, 3, cap(c.PathValues())) + }) + } +} + func TestPathValuesSizeOverMultipleRequests(t *testing.T) { e := New() e.GET("/test/:id/:action", handlerFunc) // max params is 2