diff --git a/docs/advanced-guide/routing-performance/page.md b/docs/advanced-guide/routing-performance/page.md index 5db0545e5b..9e65ac0d4f 100644 --- a/docs/advanced-guide/routing-performance/page.md +++ b/docs/advanced-guide/routing-performance/page.md @@ -44,8 +44,11 @@ Two further caveats worth setting expectations against: - **Matching is a minority of a request.** The middleware chain — tracing, logging, metrics, CORS — dominates. So end-to-end throughput moves by less than the table above, approaching it only as the route count grows. -- **The trie allocates slightly more.** Two extra allocations per matched request, for restoring the - path params and route template. This is a CPU and scaling win, not an allocation win. +- **The trie also allocates less**, where it used to allocate more. On a 100-route table, per + matched request: a static route costs 536 B / 8 allocations against the default matcher's + 960 B / 12, and a parameterised route 1208 B / 11 against 1264 B / 13. The middleware chain is + composed once per route instead of per request, and an empty path-parameter map is no longer + stored. This applies to routes registered through the framework (`app.GET`, `app.POST`, ...). ## What stays the same @@ -56,7 +59,18 @@ ordering and path cleaning all behave exactly as they do by default. Anything th — `PathPrefix` routes, static file handlers, slash-spanning parameters like `{path:.*}` — is handled by `mux` directly. Requests that match nothing are handed to `mux` in full. -Path parameters are unaffected: `ctx.PathParam("id")` and `mux.Vars(r)` work identically. +Path parameters are unaffected: `ctx.PathParam("id")` returns the same values under either +matcher. `mux.Vars(r)` returns the same values too, with one difference worth knowing: on a route +that declares no path parameters the trie leaves it `nil` where `mux` returns an empty non-nil map. +Every read behaves the same -- indexing gives the zero value, `len` is 0, and ranging does nothing -- +but an explicit `mux.Vars(r) != nil` check answers differently. + +Middleware registration becomes order-sensitive. Each route's middleware chain is composed once, +on its first request, and reused, so a middleware registered *after* a route has served does not +run for that route. Registering everything before starting the server — which is what `app.Run` +does, and what an application normally does — keeps this invisible. An application that reaches the +router itself and registers late gets an error in the log saying so rather than a middleware that +silently never runs. ## The one thing to check in your own code @@ -78,7 +92,8 @@ import gofrHTTP "gofr.dev/pkg/gofr/http" tmpl := gofrHTTP.RouteTemplate(r) // "/users/{id}", or "" if nothing matched ``` -`mux.Vars(r)` is **not** affected and needs no change. +`mux.Vars(r)` keeps working and needs no change for any ordinary read. Only an explicit nil check +against the map itself differs -- see the note above. ## Confirming which matcher is active diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index 472da0cca7..7060102c54 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -480,7 +480,7 @@ func (a *App) setupGraphQL() { } // Functional endpoint: served via POST per spec to ensure data safety and consistency. - a.httpServer.router.NewRoute().Methods(http.MethodPost).Path("/graphql").Handler(a.graphqlManager.GetHandler()) + a.httpServer.router.Add(http.MethodPost, "/graphql", a.graphqlManager.GetHandler()) } } diff --git a/pkg/gofr/http/alloc_guard_test.go b/pkg/gofr/http/alloc_guard_test.go new file mode 100644 index 0000000000..3ef14898ce --- /dev/null +++ b/pkg/gofr/http/alloc_guard_test.go @@ -0,0 +1,85 @@ +//go:build !race + +// Allocation counts are only meaningful without the race detector, which adds +// bookkeeping allocations of its own. The guard is excluded from race builds +// rather than loosened to a tolerance that would no longer catch anything. + +package http + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestTrieRequestAllocationsDoNotRegress is the guard the optimizations in this +// package lacked. +// +// Every test beside it checks behavior, which all three changes preserve by +// design -- a memoized chain, a skipped empty map and a returned slice are all +// invisible from the outside. So a revert of any of them keeps the suite green +// and gives the allocations back silently. This counts them instead. +// +// The count it pins is 7, while the commit's table quotes 8 allocs/op. They are +// different fixtures, not an inconsistency: this guard drives five middlewares +// and a nopWriter, the benchmark three and a real response writer. The guard's +// job is to notice change, so it is measured where the signal is cleanest rather +// than where the headline number comes from. +func TestTrieRequestAllocationsDoNotRegress(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + // Exact, not a ceiling. With headroom this caught only the largest of the three + // savings here: reverting the chain memoization costs five allocations and was + // caught, but the empty-Vars skip costs two and the collect signature one, and + // both slipped under a tolerance wide enough to absorb toolchain drift. + // AllocsPerRun is deterministic for fixed code, so the only thing that moves + // this is a Go or dependency upgrade -- worth a human looking at. If it fails + // after one, re-measure and update the constant in the same commit. + const wantAllocs = 7 + + r := NewRouter() + // Five middlewares, because that is what newHTTPServer installs and because the + // saving is per middleware: composing the chain per request allocates one + // closure for each. A single middleware would make the difference one + // allocation, small enough to hide under the ceiling's headroom, and the guard + // would pass with the optimization reverted. + for range 5 { + r.UseMiddleware(func(inner http.Handler) http.Handler { + // Returns a NEW handler, as every real middleware does. One that handed + // back `inner` unchanged would allocate nothing when composed, and the + // guard would then pass with the memoization reverted. + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + inner.ServeHTTP(w, req) + }) + }) + } + + r.Add(http.MethodGet, "/bench/ping", http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("ok")) })) + + // The request and writer are built once and reused, as the benchmark beside + // this does. Allocating them inside the measured closure would count + // httptest's own work -- about 17 objects -- and drown the thing being + // guarded. ServeHTTP does not mutate the request it is given; the router + // copies it when it attaches context. + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/bench/ping", http.NoBody) + w := &nopWriter{header: make(http.Header, 4)} + + // Warm the lazily built trie index and the per-route chain, so the measured + // runs see the steady state rather than one-time construction. + for range 5 { + r.ServeHTTP(w, req) + } + + got := testing.AllocsPerRun(200, func() { + r.ServeHTTP(w, req) + }) + + if got != wantAllocs { + t.Errorf("trie request path allocates %.0f objects, expected exactly %d -- an "+ + "optimization in this package has regressed, or a toolchain change moved the "+ + "baseline", got, wantAllocs) + } + + t.Logf("trie request path: %.0f allocations", got) +} diff --git a/pkg/gofr/http/chain_cache_test.go b/pkg/gofr/http/chain_cache_test.go new file mode 100644 index 0000000000..626eb98673 --- /dev/null +++ b/pkg/gofr/http/chain_cache_test.go @@ -0,0 +1,377 @@ +package http + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/gorilla/mux" + + "gofr.dev/pkg/gofr/logging" + "gofr.dev/pkg/gofr/testutil" +) + +// TestChainCacheBuildsOncePerRoute pins the optimization itself: the middleware +// CONSTRUCTORS run once per route, not once per request. +// +// This is the whole point of the cache -- each constructor returns a fresh +// closure, so running them per request allocated one object per middleware on +// every request. +func TestChainCacheBuildsOncePerRoute(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + var built atomic.Int64 + + r := NewRouter() + r.Use(func(inner http.Handler) http.Handler { + built.Add(1) + + return inner + }) + + r.Add(http.MethodGet, "/a", chainOKHandler()) + r.Add(http.MethodGet, "/b", chainOKHandler()) + + for range 20 { + serveBody(t, r, "/a") + } + + for range 20 { + serveBody(t, r, "/b") + } + + // Two routes served: two chains built, regardless of request count. + if got := built.Load(); got != 2 { + t.Errorf("middleware constructor ran %d times for 2 routes over 40 requests; want 2", got) + } +} + +// TestChainCacheRunsMiddlewareEveryRequest is the other half, and the one that +// would catch a cache that memoized too much: the middleware BODY must still run +// on every single request. A chain cached as a no-op would silently disable +// tracing, logging, metrics and CORS for every GoFr service. +func TestChainCacheRunsMiddlewareEveryRequest(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + var ran atomic.Int64 + + r := NewRouter() + r.Use(func(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + ran.Add(1) + inner.ServeHTTP(w, req) + }) + }) + r.Add(http.MethodGet, "/x", chainOKHandler()) + + const requests = 25 + for range requests { + serveBody(t, r, "/x") + } + + if got := ran.Load(); got != requests { + t.Errorf("middleware body ran %d times over %d requests; want %d", got, requests, requests) + } +} + +// TestChainCacheOrderIsPreserved pins that memoising does not reorder the chain. +// GoFr's ordering is load-bearing: Tracer must be outermost so the whole request +// sits inside its span, and Logging must wrap Metrics so both share one status +// writer. +func TestChainCacheOrderIsPreserved(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + var ( + mu sync.Mutex + order []string + ) + + record := func(name string) mux.MiddlewareFunc { + return func(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + mu.Lock() + + order = append(order, name) + mu.Unlock() + + inner.ServeHTTP(w, req) + }) + } + } + + r := NewRouter() + r.Use(record("first"), record("second"), record("third")) + r.Add(http.MethodGet, "/o", chainOKHandler()) + + // Twice: the second request is served from the cache, and must order the + // same as the first. + for range 2 { + mu.Lock() + order = order[:0] + mu.Unlock() + + serveBody(t, r, "/o") + + mu.Lock() + got := fmt.Sprint(order) + mu.Unlock() + + if want := "[first second third]"; got != want { + t.Errorf("chain order %s, want %s", got, want) + } + } +} + +// TestChainCacheIsPerRoute pins that two routes get their own chains rather than +// one route being served another's handler -- the failure a cache keyed too +// loosely would produce, and one that would route traffic to the wrong handler. +func TestChainCacheIsPerRoute(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + r := NewRouter() + r.Use(func(inner http.Handler) http.Handler { return inner }) + + for _, name := range []string{"alpha", "beta", "gamma"} { + body := name + r.NewRoute().Methods(http.MethodGet).Path("/" + name). + Handler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(body)) + })) + } + + // Twice each, interleaved, so a cache collision shows up as a wrong body. + for range 3 { + for _, name := range []string{"alpha", "beta", "gamma"} { + if got := serveBody(t, r, "/"+name); got != name { + t.Errorf("GET /%s returned %q", name, got) + } + } + } +} + +// TestChainCacheConcurrentFirstRequests pins that concurrent first requests to +// the same route do not corrupt the cache or drop middleware. The cache is +// populated on first use, so this is the window where a race would live. +func TestChainCacheConcurrentFirstRequests(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + var ran atomic.Int64 + + r := NewRouter() + r.Use(func(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + ran.Add(1) + inner.ServeHTTP(w, req) + }) + }) + r.Add(http.MethodGet, "/race", chainOKHandler()) + + const n = 64 + + var wg sync.WaitGroup + + for range n { + wg.Add(1) + + go func() { + defer wg.Done() + + if got := serveBody(t, r, "/race"); got != "ok" { + t.Errorf("body %q", got) + } + }() + } + + wg.Wait() + + if got := ran.Load(); got != n { + t.Errorf("middleware body ran %d times over %d concurrent requests; want %d", got, n, n) + } +} + +// TestMuxPathUnaffectedByChainCache pins that the default matcher still behaves +// exactly as before -- the cache lives only on the trie path. +func TestMuxPathUnaffectedByChainCache(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherMux) + + var ran atomic.Int64 + + r := NewRouter() + r.Use(func(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + ran.Add(1) + inner.ServeHTTP(w, req) + }) + }) + r.Add(http.MethodGet, "/m", chainOKHandler()) + + const requests = 10 + for range requests { + if got := serveBody(t, r, "/m"); got != "ok" { + t.Fatalf("body %q", got) + } + } + + if got := ran.Load(); got != requests { + t.Errorf("middleware body ran %d times; want %d", got, requests) + } +} + +func chainOKHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("ok")) + }) +} + +func serveBody(t *testing.T, r *Router, path string) string { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, http.NoBody) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + return w.Body.String() +} + +// TestChainCacheSkipsRoutesItDoesNotOwn pins the limit of the cache. +// +// Router embeds mux.Router publicly, so a user can register a subrouter. When +// mux matches through one it builds a fresh wrapper around the subrouter's +// handler for THAT request, while match.Route still points at the inner route -- +// so the route alone does not identify the handler. Caching on it would pin the +// first wrapper and every later request would run that one instead of its own. +// +// Only routes registered through Add, whose handler is the one Add installed and +// never changes, are eligible. +func TestChainCacheSkipsRoutesItDoesNotOwn(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + r := NewRouter() + + // Registered straight on the embedded mux.Router, exactly as a user adding a + // subrouter would: not through Add, therefore not owned. + unowned := r.NewRoute().Methods(http.MethodGet).Path("/unowned").Handler(chainOKHandler()) + + r.Add(http.MethodGet, "/owned", chainOKHandler()) + + serveBody(t, r, "/unowned") + serveBody(t, r, "/owned") + + if _, cached := r.chains.Load(unowned); cached { + t.Error("a route GoFr did not register must never enter the chain cache") + } + + var ownedCached bool + + r.chains.Range(func(_, _ any) bool { + ownedCached = true + + return false + }) + + if !ownedCached { + t.Error("a route registered through Add should be cached") + } +} + +// TestSubrouterMiddlewareRunsItsOwnInstance is the black-box form of the same +// guarantee, and the one that reproduces the user-visible defect. +// +// mux builds a fresh wrapper around a subrouter's handler for every request, +// while match.Route keeps pointing at the inner route. Caching on the route alone +// therefore pinned the FIRST request's wrapper and every later request ran that +// one instead of its own -- visible here as a sequence number frozen at 1. +func TestSubrouterMiddlewareRunsItsOwnInstance(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + r := NewRouter() + + var seq atomic.Int64 + + sub := r.PathPrefix("/api").Subrouter() + sub.Use(func(inner http.Handler) http.Handler { + // Captured per wrapper construction, so a reused wrapper reports a stale + // number while a fresh one reports its own. + n := seq.Add(1) + + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("X-Seq", strconv.FormatInt(n, 10)) + inner.ServeHTTP(w, req) + }) + }) + sub.NewRoute().Methods(http.MethodGet).Path("/thing").Handler(chainOKHandler()) + + got := make([]string, 0, 3) + + for range 3 { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/api/thing", http.NoBody)) + got = append(got, w.Header().Get("X-Seq")) + } + + if got[0] == got[1] && got[1] == got[2] { + t.Errorf("every request ran the same subrouter middleware instance (%v); the chain "+ + "cache pinned the first request's wrapper", got) + } +} + +// TestLateMiddlewareRegistrationIsReported pins that the lifecycle assumption +// announces itself when it is broken. +// +// Memoizing per route makes registration order load-bearing: a middleware added +// after a route has served never runs for that route, and nothing about the +// symptom points at the router. GoFr registers everything before Run, so this +// can only be reached by an application holding the router itself -- which is +// precisely the case that had no way to find out. +func TestLateMiddlewareRegistrationIsReported(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + out := testutil.StderrOutputForFunc(func() { + r := NewRouter() + r.UseLogger(logging.NewLogger(logging.ERROR)) + r.Add(http.MethodGet, "/ping", chainOKHandler()) + + // Before the first request there is nothing cached, so this is the ordinary + // startup path and must stay silent. + r.UseMiddleware(func(inner http.Handler) http.Handler { return inner }) + + serveBody(t, r, "/ping") + + // After it, the route's chain is fixed and this middleware will never run. + r.UseMiddleware(func(inner http.Handler) http.Handler { return inner }) + }) + + if !strings.Contains(out, "registered after the router began serving") { + t.Errorf("a middleware registered after the first request must be reported, got: %q", out) + } + + if strings.Count(out, "registered after the router began serving") != 1 { + t.Errorf("only the late registration should be reported, got: %q", out) + } +} + +// TestLateMiddlewareRegistrationSilentInMuxMode pins the other half: mux composes +// the chain per request, so a late registration takes effect there and there is +// nothing to warn about. Warning anyway would train users to ignore the message. +func TestLateMiddlewareRegistrationSilentInMuxMode(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherMux) + + out := testutil.StderrOutputForFunc(func() { + r := NewRouter() + r.UseLogger(logging.NewLogger(logging.ERROR)) + r.Add(http.MethodGet, "/ping", chainOKHandler()) + + serveBody(t, r, "/ping") + r.UseMiddleware(func(inner http.Handler) http.Handler { return inner }) + }) + + if strings.Contains(out, "registered after the router began serving") { + t.Errorf("mux mode applies late middleware, so it must not warn, got: %q", out) + } +} diff --git a/pkg/gofr/http/empty_vars_test.go b/pkg/gofr/http/empty_vars_test.go new file mode 100644 index 0000000000..7e4973eebf --- /dev/null +++ b/pkg/gofr/http/empty_vars_test.go @@ -0,0 +1,142 @@ +package http + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" +) + +// TestPathParamsSurviveEmptyVarsSkip is the compatibility proof for skipping +// mux.SetURLVars when a route has no path parameters. +// +// gorilla/mux allocates a Vars map on every successful match, so a route with no +// parameters used to store an empty map in the request context. Skipping that +// leaves mux.Vars(r) nil for those routes, and this pins that every way GoFr and +// user code reads it still behaves: parameterised routes keep every value, and +// parameter-free routes read as empty rather than panicking. +func TestPathParamsSurviveEmptyVarsSkip(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + tests := []struct { + name string + pattern string + request string + want map[string]string + }{ + {"no parameters", "/plain", "/plain", map[string]string{}}, + {"one parameter", "/user/{id}", "/user/42", map[string]string{"id": "42"}}, + {"two parameters", "/a/{x}/b/{y}", "/a/1/b/2", map[string]string{"x": "1", "y": "2"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got map[string]string + + r := NewRouter() + r.NewRoute().Methods(http.MethodGet).Path(tt.pattern). + Handler(http.HandlerFunc(func(_ http.ResponseWriter, req *http.Request) { + got = mux.Vars(req) + })) + + r.ServeHTTP(httptest.NewRecorder(), + httptest.NewRequestWithContext(t.Context(), http.MethodGet, tt.request, http.NoBody)) + + if len(got) != len(tt.want) { + t.Fatalf("mux.Vars has %d entries, want %d: %v", len(got), len(tt.want), got) + } + + for k, want := range tt.want { + if got[k] != want { + t.Errorf("mux.Vars[%q] = %q, want %q", k, got[k], want) + } + } + }) + } +} + +// TestNilVarsReadsAreSafe pins the operations GoFr and user handlers perform on +// the vars map, against the nil a parameter-free route now yields. Indexing, +// len, comma-ok and range must all behave as they did against an empty map -- +// anything else would be a panic in a running service. +func TestNilVarsReadsAreSafe(t *testing.T) { + t.Setenv(RouterEnvVar, MatcherTrie) + + r := NewRouter() + r.NewRoute().Methods(http.MethodGet).Path("/plain"). + Handler(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + + if v := vars["absent"]; v != "" { + t.Errorf("indexing yielded %q, want the zero value", v) + } + + if v, ok := vars["absent"]; ok || v != "" { + t.Errorf("comma-ok yielded (%q, %v), want (\"\", false)", v, ok) + } + + if n := len(vars); n != 0 { + t.Errorf("len is %d, want 0", n) + } + + for k := range vars { + t.Errorf("range yielded key %q over an empty map", k) + } + + w.WriteHeader(http.StatusOK) + })) + + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/plain", http.NoBody)) + + if w.Code != http.StatusOK { + t.Errorf("status %d, want 200", w.Code) + } +} + +// TestEmptyVarsSkipMatchesMuxBehaviour is the differential guard: the value a +// handler reads for a path parameter must be identical under both matchers, so +// switching GOFR_ROUTER can never change what a handler sees. +func TestEmptyVarsSkipMatchesMuxBehaviour(t *testing.T) { + for _, pattern := range []string{"/plain", "/user/{id}", "/a/{x}/b/{y}"} { + request := map[string]string{ + "/plain": "/plain", + "/user/{id}": "/user/42", + "/a/{x}/b/{y}": "/a/1/b/2", + }[pattern] + + results := make(map[string]map[string]string, 2) + + for _, matcher := range []string{MatcherMux, MatcherTrie} { + t.Setenv(RouterEnvVar, matcher) + + var got map[string]string + + r := NewRouter() + r.NewRoute().Methods(http.MethodGet).Path(pattern). + Handler(http.HandlerFunc(func(_ http.ResponseWriter, req *http.Request) { + got = mux.Vars(req) + })) + + r.ServeHTTP(httptest.NewRecorder(), + httptest.NewRequestWithContext(t.Context(), http.MethodGet, request, http.NoBody)) + + results[matcher] = got + } + + muxVars, trieVars := results[MatcherMux], results[MatcherTrie] + + if len(muxVars) != len(trieVars) { + t.Errorf("%s: mux gave %d vars, trie gave %d", pattern, len(muxVars), len(trieVars)) + continue + } + + for k, v := range muxVars { + if trieVars[k] != v { + t.Errorf("%s: vars[%q] is %q under mux and %q under trie", + pattern, k, v, trieVars[k]) + } + } + } +} diff --git a/pkg/gofr/http/middleware/tracer.go b/pkg/gofr/http/middleware/tracer.go index cc0f9964b8..f3107442ef 100644 --- a/pkg/gofr/http/middleware/tracer.go +++ b/pkg/gofr/http/middleware/tracer.go @@ -177,16 +177,22 @@ func (c headerCarrier) Set(key, value string) { http.Header(c).Set(key, value) } // this method the assertion fails and Extract silently falls back to the // single-value Get, dropping every baggage member after the first whenever a // request carries more than one Baggage header -- which is legal per W3C and is -// what proxies and service meshes commonly emit. GoFr installs -// propagation.Baggage in its default composite propagator, so that path is live. +// what proxies and service meshes commonly emit. // // Measured against the stdlib carrier with three Baggage headers: stdlib // extracted 3 members, this carrier extracted 1 before the method existed. // -// The canonical-key fast path is deliberately not used here. Baggage is not one -// of the keys canonicalPropagationKeys covers, and a carrier that replaces a -// stdlib one has to be a faithful drop-in first and an optimization second. -func (c headerCarrier) Values(key string) []string { return http.Header(c).Values(key) } +// It uses the same canonical-key table Get does. http.Header.Values +// canonicalizes whatever key it is handed, and "baggage" is not already +// canonical, so going through it allocated the canonical string on every +// request -- for the one header the propagators always ask about. +func (c headerCarrier) Values(key string) []string { + if canonical, ok := canonicalPropagationKeys[key]; ok { + return c[canonical] + } + + return http.Header(c).Values(key) +} func (c headerCarrier) Keys() []string { keys := make([]string, 0, len(c)) diff --git a/pkg/gofr/http/request_path_bench_test.go b/pkg/gofr/http/request_path_bench_test.go new file mode 100644 index 0000000000..e9b871d446 --- /dev/null +++ b/pkg/gofr/http/request_path_bench_test.go @@ -0,0 +1,144 @@ +package http + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" +) + +// BenchmarkRequestPath measures a complete request through the router and a +// realistic middleware chain, rather than the matcher alone. +// +// The matcher-only benchmark next door answers "how fast is route lookup"; this +// answers "what does a request cost", which is the number a service actually +// experiences and the one an optimization has to move. Every middleware here is +// a stand-in of the same SHAPE as the real chain -- one that wraps the writer, +// one that adds a header, one that reads the route template from the context -- +// so the per-request work being measured is the framework's plumbing rather than +// any particular middleware's body. +// +// It is deliberately in package http and free of the container, so it can be run +// on its own and gives a stable, fast signal: go test -bench RequestPath -benchmem. +func BenchmarkRequestPath(b *testing.B) { + for _, matcher := range []string{MatcherMux, MatcherTrie} { + for _, routes := range []int{10, 100, 1000} { + for _, shape := range []struct { + name string + pattern string + request string + }{ + {"static", "/bench/target", "/bench/target"}, + {"param", "/bench/target/{id}", "/bench/target/42"}, + } { + name := fmt.Sprintf("%s/%s/routes=%d", matcher, shape.name, routes) + + b.Run(name, func(b *testing.B) { + b.Setenv(RouterEnvVar, matcher) + + r := buildBenchRouter(routes, shape.pattern) + req := httptest.NewRequestWithContext(b.Context(), http.MethodGet, shape.request, http.NoBody) + w := &nopWriter{header: make(http.Header, 4)} + + // One request outside the timer so the lazy index build and any + // first-use caches are warm; a benchmark that measured them would + // be measuring startup. + r.ServeHTTP(w, req) + + b.ReportAllocs() + b.ResetTimer() + + for range b.N { + r.ServeHTTP(w, req) + } + }) + } + } + } +} + +// buildBenchRouter registers total routes with the benchmarked one registered +// LAST -- mux's worst case, and the position that makes route-table size visible. +func buildBenchRouter(total int, pattern string) *Router { + r := NewRouter() + + r.Use(writerWrappingMW, headerSettingMW, routeReadingMW) + + // Registered through Add, which is what a GoFr application uses -- app.GET and + // friends reach the router this way. Routes created straight on the embedded + // mux.Router are deliberately excluded from the middleware-chain cache, since + // their handler is not necessarily the route's own, so registering them here + // would benchmark a path no real app takes. + for i := range total - 1 { + r.Add(http.MethodGet, fmt.Sprintf("/api/v1/resource%d/{id}/action", i), benchHandler()) + } + + r.Add(http.MethodGet, pattern, benchHandler()) + + return r +} + +func benchHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Touch the vars the way a real handler does, so a change in how they are + // carried is visible here. + _ = mux.Vars(r)["id"] + + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":"ok"}`)) + }) +} + +// writerWrappingMW stands in for the logging middleware: it wraps the writer so +// the status can be read afterwards. +func writerWrappingMW(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sw := &benchStatusWriter{ResponseWriter: w} + inner.ServeHTTP(sw, r) + + _ = sw.status + }) +} + +// headerSettingMW stands in for the correlation-ID and CORS middleware. +func headerSettingMW(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header()["X-Bench-Id"] = benchHeaderValue + inner.ServeHTTP(w, r) + }) +} + +// routeReadingMW stands in for the tracer and metrics middleware, both of which +// read the matched route template out of the request on every request. +func routeReadingMW(inner http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = RouteTemplate(r) + inner.ServeHTTP(w, r) + }) +} + +//nolint:gochecknoglobals // immutable benchmark fixture. +var benchHeaderValue = []string{"bench"} + +type benchStatusWriter struct { + http.ResponseWriter + + status int +} + +func (w *benchStatusWriter) WriteHeader(code int) { + w.status = code + w.ResponseWriter.WriteHeader(code) +} + +// nopWriter is a ResponseWriter that costs nothing, so the benchmark measures +// the framework rather than httptest.ResponseRecorder's buffer growth. +type nopWriter struct { + header http.Header +} + +func (w *nopWriter) Header() http.Header { return w.header } +func (*nopWriter) Write(b []byte) (int, error) { return len(b), nil } +func (*nopWriter) WriteHeader(int) {} diff --git a/pkg/gofr/http/router.go b/pkg/gofr/http/router.go index e94352c039..b968d3c32e 100644 --- a/pkg/gofr/http/router.go +++ b/pkg/gofr/http/router.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "github.com/gorilla/mux" @@ -66,6 +67,40 @@ type Router struct { // mode. All framework registration paths go through (*Router).Use, and the // MiddlewareParity differential test guards the resulting behavior. mws []mux.MiddlewareFunc + + // chains memoises the composed middleware chain per matched route. + // + // composeMiddleware calls every middleware CONSTRUCTOR, and each one returns a + // fresh http.HandlerFunc closure. Doing that per request allocated one closure + // per middleware on every request -- five of them in a default GoFr app, which + // an allocation profile put at ~13% of all objects allocated while serving. + // The composed chain depends only on the middleware slice and the matched + // route's handler, and neither changes after startup, so it is built once per + // route and reused. + // + // Keyed on *mux.Route rather than on the handler: mux sets match.Handler from + // the route's own fixed handler field, so the route pointer identifies the + // chain exactly, and unlike a handler it is always comparable. A handler + // carrying func fields would panic as a map key. + // + // The same lifecycle assumption the trie index already makes applies here -- + // every route and every middleware is registered during startup, before the + // first request. A middleware added afterwards would not appear in a chain + // already cached for a route that had been served. + chains sync.Map + + // own records the routes registered through Add, whose handler is the one Add + // installed and never changes. Only those may use the chain cache. + own sync.Map + + // cached reports whether any chain has been memoized yet, which is to say + // whether the lifecycle assumption above has started to bite. It exists so + // Use can say so rather than leaving a middleware silently not running. + cached atomic.Bool + + // logger is optional and set by the server. Nothing on the request path uses + // it; it exists so Use can report a late registration. + logger logging.Logger } type Middleware func(handler http.Handler) http.Handler @@ -191,7 +226,17 @@ func (rou *Router) serveMatched(w http.ResponseWriter, r *http.Request, match *m // Reinstate what mux.Router.ServeHTTP would have populated so that mux.Vars(r) (used by // request.go and user handlers) and the route template (used by the tracer/metrics middleware) // keep working. - if match.Vars != nil { + // len, not nil. gorilla/mux allocates a Vars map on every successful match + // (route.go: "if match.Vars == nil { match.Vars = make(...) }"), so a route + // with no path parameters still arrives here with an empty non-nil map -- + // and storing it cost a context node and a shallow Request copy on every + // request to every parameter-free route, to carry nothing. + // + // Skipping it leaves mux.Vars(r) returning nil rather than an empty map for + // those routes. Every read stays correct: indexing a nil map yields the zero + // value, len is 0, and ranging over it does nothing -- which is what + // Request.PathParam and user handlers do with it. + if len(match.Vars) > 0 { r = mux.SetURLVars(r, match.Vars) } @@ -217,7 +262,51 @@ func (rou *Router) serveMatched(w http.ResponseWriter, r *http.Request, match *m return } - composeMiddleware(rou.mws, match.Handler).ServeHTTP(w, r) + rou.chainFor(match.Route, match.Handler).ServeHTTP(w, r) +} + +// chainFor returns the composed middleware chain for a route, building it on +// first use. See the chains field for why this is memoized and why the route is +// the key. +func (rou *Router) chainFor(route *mux.Route, h http.Handler) http.Handler { + // No route to key on: compose per request, as before. Unreachable from the + // trie matcher, which only reaches here with a matched route. + if route == nil { + return composeMiddleware(rou.mws, h) + } + + // Only routes GoFr registered itself are cached. A route reached any other way + // -- notably one under a PathPrefix(...).Subrouter(), since Router embeds + // mux.Router publicly -- can be matched with a handler mux built for THIS + // request: the subrouter wraps its handler in fresh middleware each time while + // match.Route still points at the inner route. Caching on the route alone would + // pin the first such wrapper forever and every later request would run it + // instead of its own. + if _, isOwn := rou.own.Load(route); !isOwn { + return composeMiddleware(rou.mws, h) + } + + if v, ok := rou.chains.Load(route); ok { + if composed, isHandler := v.(http.Handler); isHandler { + return composed + } + } + + composed := composeMiddleware(rou.mws, h) + + // LoadOrStore, not Store: concurrent first requests to the same route would + // otherwise each compose a chain and the last write would win, so "built once + // per route" would be true of the cache but not of the work. Whichever chain + // lands first is the one everyone uses. + actual, _ := rou.chains.LoadOrStore(route, composed) + + rou.cached.Store(true) + + if h, ok := actual.(http.Handler); ok { + return h + } + + return composed } // Use registers mux middlewares. It records them in GoFr's own chain — so the @@ -225,10 +314,42 @@ func (rou *Router) serveMatched(w http.ResponseWriter, r *http.Request, match *m // to the embedded mux router, leaving the default (mux) path unchanged. It // shadows mux.Router.Use for calls made on *Router. func (rou *Router) Use(mwf ...mux.MiddlewareFunc) { + rou.reportLateRegistration(len(mwf)) + rou.mws = append(rou.mws, mwf...) rou.Router.Use(mwf...) } +// UseLogger gives the router somewhere to report a late middleware registration. +// It is optional: a router without one behaves identically, it just cannot say +// anything. Nothing on the request path reads it. +func (rou *Router) UseLogger(l logging.Logger) { + rou.logger = l +} + +// reportLateRegistration turns a silent misconfiguration into a logged one. +// +// Memoizing the chain per route makes registration order load-bearing in trie +// mode: a middleware registered after a route has served does not appear in that +// route's cached chain and simply never runs for it, with nothing anywhere +// saying so. GoFr registers everything before Run, so this cannot fire from +// framework code -- it fires for an application that reached the router itself, +// which is exactly the case that used to debug badly. +// +// It is an error rather than a panic because the router may already be serving +// traffic, and it is trie-only: in mux mode the chain is composed per request, so +// a late registration takes effect and there is nothing to report. The atomic +// read costs nothing against a call made a handful of times at startup. +func (rou *Router) reportLateRegistration(n int) { + if n == 0 || !rou.useTrie || !rou.cached.Load() || rou.logger == nil { + return + } + + rou.logger.Errorf("%d middleware(s) registered after the router began serving: they will NOT run "+ + "for any route that has already been requested, because %s memoises each route's chain. "+ + "Register every middleware before starting the server.", n, RouterEnvVar) +} + // Matcher reports which route matcher this router uses: MatcherTrie for the // opt-in index, MatcherMux for the default linear scan. // @@ -323,7 +444,17 @@ func isDotSegment(p string, idx int) bool { // directly, avoiding the per-request child span and attribute slice grow // that an otelhttp.NewHandler wrap would add. func (rou *Router) Add(method, pattern string, handler http.Handler) { - rou.Router.NewRoute().Methods(method).Path(pattern).Handler(handler) + rou.markOwned(rou.Router.NewRoute().Methods(method).Path(pattern).Handler(handler)) +} + +// markOwned records a route whose handler is the one GoFr installed and never +// changes, making it eligible for the middleware-chain cache. +// +// Every registration GoFr makes on its own router goes through here. A route +// created directly on the embedded mux.Router does not, which is the point: see +// chainFor for why such a route must not be cached. +func (rou *Router) markOwned(route *mux.Route) { + rou.own.Store(route, struct{}{}) } // UseMiddleware registers middlewares to the router. @@ -368,7 +499,7 @@ func (rou *Router) AddStaticFiles(logger logging.Logger, endpoint, dirName strin handler := cfg.staticHandler() if endpoint == "/" { - rou.Router.NewRoute().PathPrefix(endpoint).Handler(http.StripPrefix(endpoint, handler)) + rou.markOwned(rou.Router.NewRoute().PathPrefix(endpoint).Handler(http.StripPrefix(endpoint, handler))) logger.Logf("registered static files at endpoint %v from directory %v", endpoint, absDir) @@ -380,8 +511,8 @@ func (rou *Router) AddStaticFiles(logger logging.Logger, endpoint, dirName strin // unrouted: ServeHTTP normalizes with path.Clean, which drops the trailing slash, so a request // for "/static/" arrives as "/static" and matches neither the prefix nor anything else. Register // the bare endpoint as an exact path to serve it. - rou.Router.NewRoute().Path(endpoint).Handler(http.StripPrefix(endpoint, handler)) - rou.Router.NewRoute().PathPrefix(endpoint + "/").Handler(http.StripPrefix(endpoint+"/", handler)) + rou.markOwned(rou.Router.NewRoute().Path(endpoint).Handler(http.StripPrefix(endpoint, handler))) + rou.markOwned(rou.Router.NewRoute().PathPrefix(endpoint + "/").Handler(http.StripPrefix(endpoint+"/", handler))) logger.Logf("registered static files at endpoint %v from directory %v", endpoint+"/", absDir) } diff --git a/pkg/gofr/http/trie_router.go b/pkg/gofr/http/trie_router.go index 8607c39901..d902a8bce4 100644 --- a/pkg/gofr/http/trie_router.go +++ b/pkg/gofr/http/trie_router.go @@ -300,11 +300,17 @@ func (idx *routeIndex) insert(tpl string, e *routeEntry) { // (backtracking). This is O(path length × small branching), never O(route // count). A route the trie omits here could not have matched path anyway, so // omitting it does not change the final result. -func (n *trieNode) collect(rest string, out *[]*routeEntry) { +// collect appends every route reachable along rest to out and returns the +// extended slice. +// +// It returns the slice rather than taking a *[]*routeEntry. Both shapes are +// correct, but handing a pointer-to-slice into a recursive function defeats +// escape analysis: the compiler cannot prove the callee does not retain it, so +// the caller's backing array is heap-allocated on every request. Returning the +// slice is also the ordinary Go idiom for an append-accumulator. +func (n *trieNode) collect(rest string, out []*routeEntry) []*routeEntry { if rest == "" { - *out = append(*out, n.routes...) - - return + return append(out, n.routes...) } var seg, tail string @@ -315,12 +321,14 @@ func (n *trieNode) collect(rest string, out *[]*routeEntry) { } if child, ok := n.children[seg]; ok { - child.collect(tail, out) + out = child.collect(tail, out) } if n.paramChild != nil { - n.paramChild.collect(tail, out) + out = n.paramChild.collect(tail, out) } + + return out } // match narrows candidates via the trie, adds the fallback routes, orders the @@ -337,8 +345,7 @@ func (n *trieNode) collect(rest string, out *[]*routeEntry) { func (idx *routeIndex) match(req *http.Request, rm *mux.RouteMatch) bool { var buf [12]*routeEntry - cands := buf[:0] - idx.root.collect(strings.Trim(req.URL.Path, "/"), &cands) + cands := idx.root.collect(strings.Trim(req.URL.Path, "/"), buf[:0]) // mux matches the escaped path when the router is in UseEncodedPath mode, // where the escaped and decoded forms can split into different segments. @@ -346,7 +353,7 @@ func (idx *routeIndex) match(req *http.Request, rm *mux.RouteMatch) bool { // both and take the union. Over-producing candidates is always safe (mux // filters them); missing one would not be. if esc := req.URL.EscapedPath(); esc != req.URL.Path { - idx.root.collect(strings.Trim(esc, "/"), &cands) + cands = idx.root.collect(strings.Trim(esc, "/"), cands) } if len(idx.fallback) > 0 { diff --git a/pkg/gofr/http/trie_router_test.go b/pkg/gofr/http/trie_router_test.go index a935d98b09..2e569968fc 100644 --- a/pkg/gofr/http/trie_router_test.go +++ b/pkg/gofr/http/trie_router_test.go @@ -716,7 +716,7 @@ func TestTrieRouter_IndexInvariant(t *testing.T) { // Every route mux itself would match must be among the candidates. cands := make([]*routeEntry, 0, len(idx.fallback)) - idx.root.collect(pathTrimForTest(req.URL.Path), &cands) + cands = idx.root.collect(pathTrimForTest(req.URL.Path), cands) cands = append(cands, idx.fallback...) inCandidates := make(map[*mux.Route]bool, len(cands)) diff --git a/pkg/gofr/http_server.go b/pkg/gofr/http_server.go index 2e083ad922..d235e9985c 100644 --- a/pkg/gofr/http_server.go +++ b/pkg/gofr/http_server.go @@ -75,6 +75,7 @@ func newHTTPServer(c *container.Container, port int, middlewareConfigs middlewar r := gofrHTTP.NewRouter() logRouterChoice(c.Logger, r) + r.UseLogger(c.Logger) wsManager := websocket.New()