http: use sync.Map for request-scoped vars

[mohammed90]

I received a report of a concurrent map read and map write failure at this line

caddy/modules/caddyhttp/marshalers.go

Lines 43 to 45 in ffb6ab0

	if ip, ok := GetVar(r.Context(), ClientIPVarKey).(string); ok {
	enc.AddString("client_ip", ip)
	}

The only conceivable reason for it is due to how the cache handler creates data race between cache and upstream on the same http.Request object. I cannot think of any other scenario. Whether it's truly the culprit or not, we can resolve it for all cases by using sync.Map. I contemplated creating our own opaque map struct with a mutex, but the one of use cases listed in sync.Map documentation seems to apply to our scenario:

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

Namely, the entry for a given key is written once and read many times. I searched the code base for SetVar and GetVar. I don't see cases of overwrite, but there are a couple multiple GetVar for the same key.

Assistance Disclosure

No AI was used.

[mholt]

Thanks Mohammed. This is probably fine, however I worry that it's not really solving a deeper problem... I am suspicious of the concurrent use of a Request. I worry that it may enable/encourage behavior that is actually buggy in other ways. Like, this error could be a good canary for a bug / mishandling of values.

So, I will need to think on this. I'd like to know more about the upstream use case in the cache handler.

[steadytao]

I think there are two separate concerns here.

The immediate one is that this looks like only a partial migration from map[string]any to *sync.Map. PrepareRequest() now stores *sync.Map and GetVar/SetVar were updated but VarsMiddleware, VarsMatcher, MatchVarsRE and log_append still assume map[string]any. I was able to reproduce a regression locally on this branch with go test ./modules/caddyhttp -run TestVarREMatcher -v where the {http.vars.Var1} case stops resolving because that test path still seeds VarsCtxKey with make(map[string]any) while GetVar() on this branch now only reads *sync.Map.

Separately, I share the concern that this may only be addressing one symptom. The cache-handler/Souin adapter appears to pass the original request into Souin then always forward that same original r to next.ServeHTTP while Souin runs the upstream callback through a goroutine / singleflight. That seems like a plausible source of concurrent reuse of request-scoped Caddy state so sync.Map may avoid this panic without really fixing the underlying request-sharing issue.

[mohammed90]

Yes, I tackled only 1 of the maps. This one is unexported, so it's easier to change. The others are exported directly as maps, i.e. marshalling directly onto them from JSON, and applying similar fix to them requires custom marshaller to avoid breaking existing config.

I know stdlib copies the *http.Request before passing it around. I remember Matt and I looking into their reasoning in the initial days of Caddy 2, but I don't remember if we started doing the same in Caddy. Also we don't have any disclaimers whether handlers should be goroutine-safe or not.

This is where Souin creates data race between upstream and cache:

https://github.com/darkweak/souin/blob/b0a36db1b550b66f768a3793701b69358200fed3/pkg/middleware/middleware.go#L700-L706

Maybe we should push the responsibility of ensuring thread-safety downstream? @darkweak, is it possible to re-architect this part of Souin so it relies on copies of *http.Request? Or is the revalidation race critical to the operation of cache?