diff --git a/cache/memory/memory.go b/cache/memory/memory.go index 8951b01..48a1f46 100644 --- a/cache/memory/memory.go +++ b/cache/memory/memory.go @@ -221,13 +221,42 @@ func (c *Cache) Delete(key string) { } } +// Clear removes all entries from the cache and adjusts the entry/memory +// counters to match exactly what was removed. +// +// Clear does not promise a consistent point-in-time snapshot: it takes the +// key list first, so an entry Set concurrently with Clear can survive it. +// The reverse can also happen: a concurrent Set that overwrites a key +// already collected by the Range pass can itself be deleted by Clear, +// because LoadAndDelete removes whatever mapping is current for that key +// when it runs, not necessarily the one Range observed. +// +// What Clear does guarantee is counter exactness. It LoadAndDeletes each +// collected key and subtracts exactly the MemorySize that LoadAndDelete +// returned, the same linearized pattern Delete/Get/CleanExpiredEntries/the +// evictors use, instead of the previous unconditional Delete followed by +// StoreInt64(0). That previous approach could race with a concurrent Set +// landing after the Range pass but before or after the StoreInt64 calls, +// leaving a live entry in the map while the counters read zero. The next +// Delete of that entry would then drive the counters permanently negative. func (c *Cache) Clear() { - c.entries.Range(func(key, value any) bool { - c.entries.Delete(key) + n := atomic.LoadInt64(&c.entryCount) + if n < 0 { + n = 0 + } + keys := make([]string, 0, n) + c.entries.Range(func(key, _ any) bool { + keys = append(keys, key.(string)) return true }) - atomic.StoreInt64(&c.entryCount, 0) - atomic.StoreInt64(&c.memoryUsage, 0) + + for _, key := range keys { + if removed, exists := c.entries.LoadAndDelete(key); exists { + removedEntry := removed.(CacheEntry) + atomic.AddInt64(&c.entryCount, -1) + atomic.AddInt64(&c.memoryUsage, -removedEntry.MemorySize) + } + } } func (c *Cache) CountQueries() int64 { diff --git a/cache/memory/memory_additional_test.go b/cache/memory/memory_additional_test.go index 872f4f0..df9e2ad 100644 --- a/cache/memory/memory_additional_test.go +++ b/cache/memory/memory_additional_test.go @@ -238,6 +238,88 @@ func TestMemoryCacheConcurrentSetDeleteEvict_CountersMatchActualState(t *testing } } +func TestMemoryCacheConcurrentSetDeleteClear_CountersMatchActualState(t *testing.T) { + // Regression for Clear(): the previous implementation Ranged the + // sync.Map calling Delete per key, then unconditionally reset both + // counters with StoreInt64(0). A Set racing Clear could land after the + // Range pass but before or after the StoreInt64 calls, leaving a live + // entry in the map while the counters read zero - the next Delete of + // that key then drove the counters permanently negative. + // + // Clear now follows the same linearized LoadAndDelete-then-subtract + // pattern as Delete/Get/CleanExpiredEntries/the evictors, so it no + // longer promises a consistent point-in-time snapshot (a concurrent Set + // can survive a Clear) but does guarantee entryCount/memoryUsage always + // match the entries actually removed. As in + // TestMemoryCacheConcurrentSetDeleteEvict_CountersMatchActualState, + // which key wins a given race is nondeterministic, so the assertion + // recomputes "expected" from the final map contents rather than + // asserting a fixed number. + const ( + numKeys = 50 + numWorkers = 16 + numOpsEach = 300 + ) + + cache := NewWithSize(DefaultTestExpiration, DefaultMaxMemorySize, DefaultMaxCacheSize) + defer cache.Shutdown() + + keyFor := func(i int) string { return fmt.Sprintf("clear-key-%d", i) } + + // Pre-seed so Clear and the Delete workers have entries to act on from + // the start. + for i := range numKeys { + cache.Set(keyFor(i), []byte("seed-value"), DefaultTestExpiration) + } + + var wg sync.WaitGroup + for w := range numWorkers { + wg.Add(1) + go func(worker int) { + defer wg.Done() + for op := range numOpsEach { + k := keyFor((worker + op) % numKeys) + switch op % 2 { + case 0: + cache.Set(k, []byte("value"), DefaultTestExpiration) + case 1: + cache.Delete(k) + } + } + }(w) + } + + wg.Add(1) + go func() { + defer wg.Done() + for range 100 { + cache.Clear() + } + }() + + wg.Wait() + + // Stop the background cleanup goroutine before recomputing expected + // state, so it cannot race with the Range below (nothing has expired + // yet - DefaultTestExpiration is 5s - but this removes any doubt). + cache.Shutdown() + + var wantCount, wantMemory int64 + cache.entries.Range(func(_, v any) bool { + entry := v.(CacheEntry) + wantCount++ + wantMemory += entry.MemorySize + return true + }) + + if got := cache.CountQueries(); got != wantCount { + t.Fatalf("entryCount = %d, want %d (actual live entries after join)", got, wantCount) + } + if got := cache.GetMemoryUsage(); got != wantMemory { + t.Fatalf("memoryUsage = %d, want %d (sum of live entries' MemorySize after join)", got, wantMemory) + } +} + func TestMemoryCacheClose_IdempotentAndStillUsable(t *testing.T) { // Compile-time guard: the memory backend must satisfy io.Closer so // cache.Shutdown() can stop its cleanup goroutine (it otherwise leaks