Why does a Go middleware's map of per-client rate limiters, filled on first request, break under concurrent load?
answer
- how many goroutines serve requests here?
- the map is written on first sight
- not a panic you can recover
- read lock then write lock is not atomic
- one lock across lookup and insert
basics
~20 sA Go server runs every request on its own goroutine, so an unguarded map is read and written concurrently. The runtime throws a fatal error on concurrent map writes, which recover cannot catch. Guard lookup and insert with one mutex.
solid answer
~50 s`net/http` serves each request on its own goroutine, so the middleware's map is touched by many goroutines at once. A plain Go map is not safe for concurrent use when any goroutine writes, and this map is written every time a new client appears. The runtime's map has a cheap concurrency check, so you get `fatal error: concurrent map writes` or `concurrent map read and map write` — a throw no deferred `recover` can stop, which takes the whole process down. The fix is one `sync.Mutex` held across the whole lookup-or-create, not just the insert: with a read lock for the lookup and a write lock for the insert, two goroutines can both miss and both construct a limiter, and the second overwrites the first, handing that client a fresh full burst. Prove it with `go test -race` driving concurrent requests through the handler.
code
go · 23 linestype key struct{ client, route string }
type visitor struct {
limiter *rate.Limiter
lastSeen time.Time
}
type limiters struct {
mu sync.Mutex
m map[key]*visitor
}
func (l *limiters) get(k key) *visitor {
l.mu.Lock()
defer l.mu.Unlock()
v, ok := l.m[k]
if !ok {
v = &visitor{limiter: newLimiter()} // one bucket per client+route
l.m[k] = v
}
v.lastSeen = time.Now()
return v
}go deeper
Know that every request runs on its own goroutine and that a Go map cannot be written by two goroutines at once. Being able to add a sync.Mutex around the lookup and insert is what is expected here.
Explain the exact failure — a fatal runtime throw for concurrent map writes, not a recoverable panic — and why the lookup and the insert must be inside one critical section rather than two. Mention sync.Map's LoadOrStore as the alternative and what it costs.
Demonstrate how you would prove it: a -race test driving real concurrency through the handler, and the reasoning that a clean run only clears the interleavings exercised. Talk about lock scope — the limiter call belongs outside the critical section.
Frame it as a review standard rather than a fix: shared mutable state in middleware is the highest-value thing to gate in code review and to cover with a race-enabled CI job, because this failure kills the process rather than one request.
## Why the map is shared at all A per-client limiter needs somewhere to live between requests, because the whole point is that the bucket carries state from one request to the next. The usual shape is a map from a client identity — an API key, a normalised remote address, or a small comparable struct combining the client and the route — to that client's limiter, created lazily the first time that client appears. That map is package- or struct-level state shared by every request. `net/http` accepts each connection and serves each request on its own goroutine, and it does so with no serialisation between handlers whatsoever: two requests from the same client on two connections run at the same instant, and a thousand requests from a thousand clients run at the same instant. So the map is concurrently accessed by construction. ## What Go actually does Go's built-in map is not safe for concurrent use if any goroutine writes to it. Concurrent reads alone are fine. As soon as one goroutine writes while another reads or writes, the program has a data race, and the behaviour is undefined: a lookup can return garbage, an insert can corrupt the table, and the value you get back may be a pointer into memory that is being rearranged. Because that class of bug is so common and so destructive, the runtime keeps a lightweight flag on the map and *throws* when it notices overlapping operations. The message is `fatal error: concurrent map writes` or `fatal error: concurrent map read and map write`. This is important to state precisely in an interview: it is a **runtime throw, not a panic**. A deferred `recover`, including the one your recovery middleware installs, cannot catch it. The process dies and takes every in-flight request with it. The check is also opportunistic — it catches many races but is not a guarantee, so a program can run for weeks and then die under a traffic spike. ## The fix, and the subtler bug inside the fix The correct guard is one mutex held across the **entire lookup-or-create**: 1. lock; 2. look the key up; 3. if it is missing, construct the limiter and store it; 4. touch any bookkeeping (a `lastSeen` timestamp); 5. unlock, and use the limiter. The common half-fix is to reach for `sync.RWMutex` on the theory that lookups dominate: take `RLock` for the read, and if the key is missing, drop it and take `Lock` to insert. Between releasing the read lock and taking the write lock, another goroutine can insert the same key. If your insert does not re-check under the write lock, it overwrites a limiter that already has spend recorded against it with a fresh one at full burst. A client that sends two simultaneous first requests — the normal shape of a browser or a parallel client — can keep resetting its own bucket. The rule is: whatever lock you use, re-check the key after acquiring the write lock, and only insert if it is still missing. `sync.Map` is a legitimate alternative for this access pattern, and `LoadOrStore` collapses the check-and-insert into one atomic step, discarding the loser's freshly built limiter rather than installing it. What it costs you is bookkeeping: there is no length, and iterating to expire entries is clumsier than over a plain map you already hold a lock on. For a limiter map that you also need to sweep, a plain map behind a mutex is usually the simpler correct answer, and the mutex is held for a handful of nanoseconds. Holding the lock while *using* the limiter is a different mistake: take the lock only to get the limiter out, release it, then ask the limiter for permission. The limiter itself is safe for concurrent use. ## Proving it The diagnostic is the race detector. Build the test binary with `go test -race`, stand the handler up with `httptest`, and fire many concurrent requests at it — a mix of repeated keys and fresh keys, so both the read path and the insert path are exercised. Give each goroutine its own `httptest.NewRecorder`, since a recorder is not itself safe to share. The detector reports the two conflicting accesses with both stacks, which is exactly the evidence a code review argument needs. Two caveats worth stating. First, `-race` only finds races on memory that the run actually touches concurrently, so a clean run proves nothing about the paths your test did not drive — you must generate real overlap, not a sequential loop. Second, a `-race` binary is several times slower and much hungrier for memory, so it belongs in CI and in load tests, not in production.
- Two goroutines miss the map for the same key at once. What can the client gain?If the insert does not re-check under the write lock, the second store replaces a limiter that has already spent tokens with a fresh one at full burst — so a client sending parallel first requests keeps resetting its own bucket and outruns its limit. Re-check the key after taking the write lock and insert only if it is still absent, or use `sync.Map`'s `LoadOrStore`.
- Would sync.Map remove the problem?It removes the data race and, with `LoadOrStore`, the double-insert too — the loser's limiter is simply discarded. The cost is bookkeeping: no length, and expiring stale entries means ranging over it rather than sweeping a map you already hold a lock on. For a limiter map you also need to prune, a plain map behind a `sync.Mutex` is usually simpler.
- How do you make the race show up in a test rather than in production?Run `go test -race` with a test that drives genuinely concurrent requests through the handler — many goroutines, a mix of repeated and fresh client keys, each with its own `httptest.NewRecorder`. The detector prints both conflicting stacks. Remember a clean run only clears the interleavings that actually happened, so the test has to create real overlap.
- Should the middleware hold the mutex while asking the limiter for permission?No. Hold it only long enough to look up or create the entry, then release it and call the limiter outside the critical section. The limiter is safe for concurrent use itself, and holding a single global mutex across the permission check serialises every request in the server through one lock for no benefit.
saying these in an interview costs you the question
- Says concurrent map reads and writes are fine if writes are rare
- Claims a deferred recover can catch concurrent map writes
- Uses a read lock around the create branch without re-checking
- Assumes net/http serialises handlers per client or per path
- Holds the mutex while calling the limiter for every request
- Believes a passing -race run proves the code is race-free