review: fix nested c.JSON writer reuse, unexport sonic API, add pooling/dispatch tests

Addresses PR review findings:
- context: guard delayedStatusWriter reuse against re-entrant c.JSON (avoid self-reference);
  document the unsafe stringToBytes contract (string lifetime + no-retain by wrapping writers)
- sonic: unexport Serializer.API (constructor-only; nil-safe via cfg()) before release
- tests: store no-leak across Reset, JSON status across Reset, nested c.JSON, global/pre
  middleware on 404/405/OPTIONS, randomString concurrency (-race), sonic bad-JSON + indent
  + zero-value serializer, query fast-path malformed-escape cases
- docs: keep-in-sync notes on bind tag cache; stamp sonic benchmark date/version

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: vishr

bind.go

@@ -141,15 +141,21 @@ func (b *DefaultBinder) Bind(c *Context, target any) error {
 // once per struct type and reuse it across requests (see bindStructMeta). Only type-level data is cached here;
 // per-request, per-instance reflect.Value operations still happen in bindData.
 type bindFieldMeta struct {
-	index     int          // field index within the struct
-	anonymous bool         // reflect.StructField.Anonymous
-	fieldKind reflect.Kind // declared field type kind (typeField.Type.Kind())
-	formatTag string       // value of the `format` struct tag
-	// binding-source tag values; bindData is only ever called with one of these four tags.
+	index int // field index within the struct
+	// fieldKind is the DECLARED field kind (typeField.Type.Kind()), used only for unmarshal dispatch.
+	// It is intentionally not the post-anonymous-pointer-deref live kind; bindData computes that
+	// separately as structFieldKind where needed.
+	fieldKind reflect.Kind
+	anonymous bool   // reflect.StructField.Anonymous
+	formatTag string // value of the `format` struct tag
+	// binding-source tag values. bindData is only ever called with one of these four tags (see the
+	// callers BindPathValues/BindQueryParams/BindBody/BindHeaders). Keep these fields, the four
+	// f.Tag.Get(...) lines in bindMetaFor, and the tagName switch in sync if a source is ever added.
 	param, query, form, header string
 }
 
 // tagName returns the field's tag value for the given binding source tag.
+// Keep in sync with the tag fields above and the f.Tag.Get calls in bindMetaFor.
 func (m *bindFieldMeta) tagName(tag string) string {
 	switch tag {
 	case "param":

context.go

@@ -22,10 +22,15 @@ import (
 	"unsafe"
 )
 
-// stringToBytes returns a []byte view over s without copying. The result MUST only be passed to APIs that
-// treat the slice as read-only and do not retain it (e.g. http.ResponseWriter.Write, which copies as needed).
-// This avoids the allocation+copy of []byte(s) on the response write path. See findings on zero-copy writes
-// used by fasthttp/fiber. Writing through the returned slice is undefined behavior.
+// stringToBytes returns a []byte view over s without copying, avoiding the allocation+copy of []byte(s)
+// on the response write path (the zero-copy technique used by fasthttp/fiber).
+//
+// Contract — all of the following must hold at every call site:
+//   - The result is read-only: writing through it is undefined behaviour.
+//   - The callee must NOT retain the slice beyond the call. It is only passed to the response
+//     Writer's Write, whose io.Writer contract forbids retaining/mutating the argument. Note the
+//     concrete writer may be a wrapping ResponseWriter (e.g. gzip); such writers must copy, not alias.
+//   - s must stay reachable for as long as the slice is used: the slice aliases s's backing array.
 func stringToBytes(s string) []byte {
 	if s == "" {
 		return nil
@@ -543,9 +548,16 @@ func (c *Context) json(code int, i any, indent string) error {
 	// (global) error handler decides correct status code for the error to be sent to the client.
 	// For that we need to use writer that can store the proposed status code until the first Write is called.
 	resp := c.Response()
-	// Reuse the Context-owned delayedStatusWriter instead of heap-allocating one per JSON response.
-	c.dsw = delayedStatusWriter{ResponseWriter: resp, status: code}
-	c.SetResponse(&c.dsw)
+	// Reuse the Context-owned delayedStatusWriter to avoid heap-allocating one per JSON response.
+	// If we are already nested inside a delayed write (rare: a serializer or handler calling c.JSON
+	// re-entrantly), allocate a fresh writer so the outer call's writer (which is &c.dsw) is not
+	// clobbered — reusing c.dsw here would make it reference itself.
+	if _, nested := resp.(*delayedStatusWriter); nested {
+		c.SetResponse(&delayedStatusWriter{ResponseWriter: resp, status: code})
+	} else {
+		c.dsw = delayedStatusWriter{ResponseWriter: resp, status: code}
+		c.SetResponse(&c.dsw)
+	}
 	defer c.SetResponse(resp)
 
 	return c.echo.JSONSerializer.Serialize(c, i, indent)

dispatch_pool_test.go

@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: MIT
+// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors
+
+package echo
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestContextResetClearsStore guards the clear(c.store) reuse: a pooled Context must not leak store
+// values from a previous request into the next one, and Set must still work after a clear-based Reset.
+func TestContextResetClearsStore(t *testing.T) {
+	e := New()
+	c := e.NewContext(httptest.NewRequest(http.MethodGet, "/", nil), httptest.NewRecorder())
+	c.Set("secret", "req1")
+	assert.Equal(t, "req1", c.Get("secret"))
+
+	c.Reset(httptest.NewRequest(http.MethodGet, "/", nil), httptest.NewRecorder())
+	assert.Nil(t, c.Get("secret"), "store must not leak across Reset")
+
+	c.Set("k", "req2") // Set must still work after clear-based reset
+	assert.Equal(t, "req2", c.Get("k"))
+}
+
+// TestContextJSONStatusAcrossReset guards the reused delayedStatusWriter (c.dsw): a second JSON
+// response on a pooled+Reset Context must use the new status, not inherit the previous one.
+func TestContextJSONStatusAcrossReset(t *testing.T) {
+	e := New()
+	c := e.NewContext(httptest.NewRequest(http.MethodGet, "/", nil), httptest.NewRecorder())
+	assert.NoError(t, c.JSON(http.StatusTeapot, map[string]int{"a": 1}))
+
+	rec2 := httptest.NewRecorder()
+	c.Reset(httptest.NewRequest(http.MethodGet, "/", nil), rec2)
+	assert.NoError(t, c.JSON(http.StatusCreated, map[string]int{"b": 2}))
+	assert.Equal(t, http.StatusCreated, rec2.Code)
+	assert.JSONEq(t, `{"b":2}`, rec2.Body.String())
+}
+
+// TestNestedJSONUsesFreshDelayedWriter guards the nested c.JSON case: a serializer that calls c.JSON
+// re-entrantly must not corrupt the outer delayedStatusWriter (regression test for c.dsw self-reference).
+func TestNestedJSONUsesFreshDelayedWriter(t *testing.T) {
+	e := New()
+	e.JSONSerializer = nestedJSONSerializer{}
+	rec := httptest.NewRecorder()
+	c := e.NewContext(httptest.NewRequest(http.MethodGet, "/", nil), rec)
+	assert.NoError(t, c.JSON(http.StatusOK, map[string]int{"outer": 1}))
+	assert.Equal(t, http.StatusOK, rec.Code)
+}
+
+type nestedJSONSerializer struct{}
+
+func (nestedJSONSerializer) Serialize(c *Context, i any, indent string) error {
+	if m, ok := i.(map[string]int); ok && m["outer"] == 1 {
+		// re-enter c.JSON once before encoding the outer payload
+		if err := c.JSON(http.StatusOK, map[string]int{"inner": 2}); err != nil {
+			return err
+		}
+	}
+	return (DefaultJSONSerializer{}).Serialize(c, i, indent)
+}
+
+func (nestedJSONSerializer) Deserialize(c *Context, i any) error {
+	return (DefaultJSONSerializer{}).Deserialize(c, i)
+}
+
+// TestGlobalMiddlewareRunsOnNotFoundAndMethodNotAllowed pins the dispatch contract: global (Use) and
+// pre (Pre) middleware must execute even when the router returns 404 / 405 / OPTIONS handlers.
+func TestGlobalMiddlewareRunsOnNotFoundAndMethodNotAllowed(t *testing.T) {
+	cases := []struct {
+		name, method, path string
+		code               int
+	}{
+		{"404", http.MethodGet, "/missing", http.StatusNotFound},
+		{"405", http.MethodPost, "/", http.StatusMethodNotAllowed},
+		{"OPTIONS", http.MethodOptions, "/", http.StatusNoContent},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := New()
+			var pre, use bool
+			e.Pre(func(n HandlerFunc) HandlerFunc {
+				return func(c *Context) error { pre = true; return n(c) }
+			})
+			e.Use(func(n HandlerFunc) HandlerFunc {
+				return func(c *Context) error { use = true; return n(c) }
+			})
+			e.GET("/", func(c *Context) error { return c.String(http.StatusOK, "ok") })
+
+			rec := httptest.NewRecorder()
+			e.ServeHTTP(rec, httptest.NewRequest(tc.method, tc.path, nil))
+
+			assert.True(t, pre, "pre-middleware must run on %s", tc.name)
+			assert.True(t, use, "global middleware must run on %s", tc.name)
+			assert.Equal(t, tc.code, rec.Code)
+		})
+	}
+}

middleware/randomstring_concurrent_test.go

@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: MIT
+// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors
+
+package middleware
+
+import (
+	"strings"
+	"sync"
+	"testing"
+)
+
+// TestRandomStringConcurrent guards the pooled scratch buffers in randomString: concurrent callers
+// must not share/alias a buffer and corrupt each other's output. Run with -race.
+func TestRandomStringConcurrent(t *testing.T) {
+	const goroutines, iterations = 100, 300
+	var wg sync.WaitGroup
+	wg.Add(goroutines)
+	for g := 0; g < goroutines; g++ {
+		go func() {
+			defer wg.Done()
+			for i := 0; i < iterations; i++ {
+				s := randomString(32)
+				if len(s) != 32 {
+					t.Errorf("expected length 32, got %d (%q)", len(s), s)
+					return
+				}
+				for _, r := range s {
+					if !strings.ContainsRune(randomStringCharset, r) {
+						t.Errorf("char %q not in charset (%q)", r, s)
+						return
+					}
+				}
+			}
+		}()
+	}
+	wg.Wait()
+}

query_fastpath_test.go

@@ -41,6 +41,8 @@ func TestGetRawQueryParamMatchesStdlib(t *testing.T) {
 		"a=x+y",      // '+' -> space
 		"a=%20space", // percent escape
 		"a=%2",       // bad value escape -> pair skipped
+		"a=%ZZ&a=2",  // bad escape on first match -> skip, fall through to second
+		"%ZZ=1&a=2",  // bad key escape -> pair skipped
 		"a%3Db=c",    // encoded key 'a=b'
 		"a=1;b=2",    // semicolon segment skipped entirely
 		"a=1&c=3;d=4&e=5",

sonic/README.md

@@ -55,8 +55,10 @@ workload, but as a rule of thumb:
 
 ### Benchmarks
 
-End-to-end through `ServeHTTP`, Apple M3 Max (arm64), Go 1.26, representative payload
-(struct with strings, ints, bool, slices, floats and a map):
+End-to-end through `ServeHTTP`, Apple M3 Max (arm64), Go 1.26, sonic v1.15.2 (measured 2026-06),
+representative payload (struct with strings, ints, bool, slices, floats and a map). Absolute numbers
+are illustrative and will drift across Go/sonic versions and hardware — the **direction** is the
+durable takeaway:
 
 | Operation | `encoding/json` | sonic | Δ time | Δ allocs |
 |---|--:|--:|--:|--:|

sonic/sonic.go

@@ -25,22 +25,33 @@ import (
 	"github.com/labstack/echo/v5"
 )
 
-// Serializer implements echo.JSONSerializer using the sonic library.
+// Serializer implements echo.JSONSerializer using the sonic library. Construct it with New or
+// NewWithAPI; the underlying sonic configuration is intentionally not an exported field so the
+// serializer is safe for concurrent use and cannot be misconfigured to a nil config.
 type Serializer struct {
-	// API is the frozen sonic configuration used for encoding/decoding.
-	// Defaults to sonic.ConfigDefault when created via New.
-	API sonic.API
+	// api is the frozen sonic configuration used for encoding/decoding. A frozen sonic.API is safe
+	// for concurrent use, which echo.JSONSerializer requires.
+	api sonic.API
 }
 
 // New returns a Serializer using sonic's default configuration.
 func New() *Serializer {
-	return &Serializer{API: sonic.ConfigDefault}
+	return &Serializer{api: sonic.ConfigDefault}
 }
 
 // NewWithAPI returns a Serializer using the provided sonic configuration, e.g.
 // sonic.ConfigStd (encoding/json compatible) or sonic.ConfigFastest.
 func NewWithAPI(api sonic.API) *Serializer {
-	return &Serializer{API: api}
+	return &Serializer{api: api}
+}
+
+// cfg returns the configured sonic API, falling back to sonic.ConfigDefault so a zero-value
+// Serializer (constructed without New/NewWithAPI) does not panic.
+func (s *Serializer) cfg() sonic.API {
+	if s.api == nil {
+		return sonic.ConfigDefault
+	}
+	return s.api
 }
 
 // Serialize converts target into JSON and writes it to the response.
@@ -55,9 +66,9 @@ func (s *Serializer) Serialize(c *echo.Context, target any, indent string) error
 		err error
 	)
 	if indent != "" {
-		buf, err = s.API.MarshalIndent(target, "", indent)
+		buf, err = s.cfg().MarshalIndent(target, "", indent)
 	} else {
-		buf, err = s.API.Marshal(target)
+		buf, err = s.cfg().Marshal(target)
 	}
 	if err != nil {
 		return err
@@ -68,7 +79,7 @@ func (s *Serializer) Serialize(c *echo.Context, target any, indent string) error
 
 // Deserialize reads JSON from the request body and stores it in target.
 func (s *Serializer) Deserialize(c *echo.Context, target any) error {
-	if err := s.API.NewDecoder(c.Request().Body).Decode(target); err != nil {
+	if err := s.cfg().NewDecoder(c.Request().Body).Decode(target); err != nil {
 		return echo.ErrBadRequest.Wrap(err)
 	}
 	return nil

sonic/sonic_test.go

@@ -5,6 +5,7 @@ package sonic
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -82,6 +83,55 @@ func TestSerializerRoundTrip(t *testing.T) {
 	}
 }
 
+// TestDeserializeBadJSON verifies malformed input is mapped to a 400 echo.HTTPError, matching the
+// default serializer's contract that BindBody relies on.
+func TestDeserializeBadJSON(t *testing.T) {
+	e := echo.New()
+	c := e.NewContext(httptest.NewRequest(http.MethodPost, "/", bytes.NewReader([]byte("{bad"))), httptest.NewRecorder())
+	var out payload
+	err := New().Deserialize(c, &out)
+	if err == nil {
+		t.Fatal("expected error for malformed JSON")
+	}
+	var he *echo.HTTPError
+	if !errors.As(err, &he) {
+		t.Fatalf("expected *echo.HTTPError, got %T", err)
+	}
+	if he.Code != http.StatusBadRequest {
+		t.Fatalf("expected 400, got %d", he.Code)
+	}
+}
+
+// TestSerializeIndent exercises the indent (MarshalIndent) branch used by c.JSONPretty.
+func TestSerializeIndent(t *testing.T) {
+	e := echo.New()
+	rec := httptest.NewRecorder()
+	c := e.NewContext(httptest.NewRequest(http.MethodGet, "/", nil), rec)
+	if err := New().Serialize(c, sample(), "  "); err != nil {
+		t.Fatalf("Serialize with indent: %v", err)
+	}
+	if !bytes.Contains(rec.Body.Bytes(), []byte("\n  ")) {
+		t.Fatalf("expected indented output, got %q", rec.Body.String())
+	}
+	var out payload
+	if err := New().Deserialize(e.NewContext(httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(rec.Body.Bytes())), httptest.NewRecorder()), &out); err != nil {
+		t.Fatalf("indented output did not round-trip: %v", err)
+	}
+	if !reflect.DeepEqual(sample(), out) {
+		t.Fatalf("indent round trip mismatch: %#v", out)
+	}
+}
+
+// TestZeroValueSerializer ensures a zero-value Serializer (no constructor) does not panic.
+func TestZeroValueSerializer(t *testing.T) {
+	e := echo.New()
+	rec := httptest.NewRecorder()
+	c := e.NewContext(httptest.NewRequest(http.MethodGet, "/", nil), rec)
+	if err := (&Serializer{}).Serialize(c, sample(), ""); err != nil {
+		t.Fatalf("zero-value Serialize: %v", err)
+	}
+}
+
 func benchSerialize(b *testing.B, s echo.JSONSerializer) {
 	e := echo.New()
 	in := sample()