Add ability to poison contexts, preventing reuse

[BlakeWilliams]

I’m working with several teams to extract new services, replacing existing API endpoints one by one. To validate these changes we're comparing responses from the legacy ("control") service against the new ("candidate") service. We initially started implementing this in chi, but ran into race conditions due to sync.Pool reusing chi.Context between requests. This is due to us returning the "control" response immediately, followed by calling the "candidate" response in the background (which utilizes context) outside of the original req/res lifecycle.

We have some workarounds in place, but ideally chi would allow more control over context reuse for this use-case. I took a pretty basic swing at this, implementing a new PreventReuse() method on Context that marks the Context as poisoned and avoiding the sync.Pool.Put call appropriately.

I totally understand if this is out of scope or isn't an API you wanted to support, but I wanted to open a PR to get feedback and see if there's interest instead of assuming.

Either way, the good news here is that the new services are using chi. ;)

[VojtechVitek]

Hi @BlakeWilliams,

Can you share an example reproducer, so we understand the the issue you're running into?

Are you keeping chi.Context around in a new goroutine outside of the Request scope?

[BlakeWilliams]

@VojtechVitek It's a lot of code tied to internal libraries but I can try to make a simple repro case for it.

Are you keeping chi.Context around in a new goroutine outside of the Request scope?

Yes, we are. We use the same chi.Context to serve the request from "control" proxy which returns immediately (so the framework calls sync.Pool.Put while in a background goroutine we're using the same context to run the "candidate" proxy.

[VojtechVitek]

@BlakeWilliams can you make a copy of the chi.Context, so you can use it outside of the request goroutine scope?

rctx := r.Context().Value(chi.RouteCtxKey).(*Context)

copy := chi.NewRouteContext()
*copy = *rctx

[BlakeWilliams]

@VojtechVitek we discussed that approach, but the concern is that chi could change the underlying behavior at any point that makes copying the struct unsafe, since this isn't publicly documented behavior.

In general, re-using request scoped resources is tricky and error prone, so we're exercising a lot of caution here. (see Ruby's unicorn web server https://yhbt.net/unicorn-public/66A68DD8-83EF-4C7A-80E8-3F1F7AB31670@github.com and how its env re-use resulted in problematic race-conditions).

[BlakeWilliams]

Here's a small repro that's close enough to what we're doing:

// main.go
package main

import (
	"context"
	"fmt"
	"log"
	"net/http"
	"time"

	"github.com/go-chi/chi/v5"
)

var request int

type Result struct {
	Status int
	Body   []byte
}

type Experiment struct {
	Control   func(context.Context) Result
	Candidate func(context.Context) Result
}

func NewExperiment(control, candidate func(context.Context) Result) *Experiment {
	return &Experiment{Control: control, Candidate: candidate}
}

func (e *Experiment) Run(ctx context.Context) Result {
	ctrl := e.Control(ctx)

	go func(ctrl Result) {
		// Exagerated delay to help showcase the race
		time.Sleep(500 * time.Millisecond)
		_ = e.Candidate(ctx)

		// log difference
	}(ctrl)

	return ctrl
}

func helloHandler(w http.ResponseWriter, r *http.Request) {
	name := chi.URLParam(r, "name") // captured before response returns

	rc := chi.RouteContext(r.Context())
	routePath := rc.RoutePattern()

	rc.PreventReuse()

	exp := NewExperiment(
		func(ctx context.Context) Result {
			return Result{
				Status: http.StatusOK,
				Body:   []byte("control hello " + name + "\n"),
			}
		},
		func(ctx context.Context) Result {
			rc := chi.RouteContext(r.Context())
			candidateRoutePath := rc.RoutePattern()

			if routePath != candidateRoutePath {
				fmt.Println("route paths do not match:", routePath, candidateRoutePath)
			}
			return Result{
				Status: http.StatusOK,
				Body:   []byte("candidate hello " + name + "\n"),
			}
		},
	)

	ctrl := exp.Run(r.Context())

	w.WriteHeader(ctrl.Status)
	_, _ = w.Write(ctrl.Body)
}

func main() {
	r := chi.NewRouter()

	r.Get("/hello/{name}", helloHandler)
	r.Get("/goodbye/{name}", helloHandler)

	addr := ":3005"
	log.Printf("listening on %s", addr)
	if err := http.ListenAndServe(addr, r); err != nil {
		log.Fatal(err)
	}
}

For main remove rc.PreventReuse(), but if running on this branch keep it to see the error message no longer being logged. I was running this to get the mismatch logs: for i in {1..5}; do curl localhost:3005/hello/world & curl localhost:3005/goodbye/context; done; wait

[VojtechVitek]

Let's go for rctx := r.Context().Value(chi.RouteCtxKey).(*Context).

Perhaps we can introduce a .Clone() method on the Context type. Would that work for you?