Code review: a high-load in-memory cache using sync.Mutex — find the concurrency bugs
Review this in-memory cache meant to run under high load in production, with a read/write ratio of roughly 80/20. GetOrCreate should return the existing value for a key or store and return the new one; Get should return the cached value. Identify every concurrency and correctness bug and say how you would fix them.
var cache = make(map[string]string)
// GetOrCreate checks whether key exists; if not, it creates a new value.
func GetOrCreate(key, value string) string {
var m sync.Mutex
m.Lock()
value = cache[key]
m.Unlock()
if value != "" {
return value
}
m.Lock()
cache[key] = value
m.Unlock()
return value
}
func Get(key string) string {
var m sync.Mutex
m.Lock()
v := cache[key]
m.Unlock()
return v
}Find and fix the bugs.
The sync.Mutex is a local variable, so every call locks its own copy and the shared cache is never protected — concurrent calls race and crash with fatal error: concurrent map writes. Also value = cache[key] overwrites the argument, so the create path stores "", and unlocking between read and write makes get-or-create non-atomic. Fix: one shared lock — an RWMutex since reads dominate — held across the whole check-then-set, and stop clobbering value.
- ✗Not noticing the mutex is a local variable, so it synchronizes nothing across goroutines
- ✗Missing that
value = cache[key]overwrites the argument, so the create path stores an empty string - ✗Treating the separate lock for read and for write as if the get-or-create were atomic
- →Why does taking an
RLockfor the read then aLockfor the write still let two callers both create a value? - →How would
sync.Mapor asingleflightgroup change this design?
Find the bugs
var cache = make(map[string]string)
func GetOrCreate(key, value string) string {
var m sync.Mutex // ❌ local mutex — each call gets its own
m.Lock()
value = cache[key] // ❌ clobbers the value argument
m.Unlock()
if value != "" {
return value
}
m.Lock()
cache[key] = value // ❌ value is already "" — stores an empty string
m.Unlock() // ❌ lock released between read and write
return value
}Why it is broken
- The mutex is local.
var m sync.Mutexis declared inside the function, so every call takes its own mutex. The sharedcacheis protected by nothing — concurrent calls write the map at once and abort:
fatal error: concurrent map writes- The argument is clobbered.
value = cache[key]overwrites the passed value with the lookup result. On a miss,valuebecomes"", and the store below saves an empty string rather than what the caller asked for.
- Get-or-create is not atomic. The lock is dropped between the read and the write, so two calls can both miss and both create a value — the "check" and the "set" are not one unit.
✅ Fix — one shared RWMutex (reads are 80%), held across the whole check-then-set, with , ok to distinguish a missing key from an empty value:
var (
mu sync.RWMutex
cache = make(map[string]string)
)
func GetOrCreate(key, value string) string {
mu.Lock()
defer mu.Unlock()
if existing, ok := cache[key]; ok {
return existing
}
cache[key] = value
return value
}
func Get(key string) string {
mu.RLock()
defer mu.RUnlock()
return cache[key]
}
