diff --git a/src/x/cache/lru_cache.go b/src/x/cache/lru_cache.go index 81e7f26995..6ace3f11d6 100644 --- a/src/x/cache/lru_cache.go +++ b/src/x/cache/lru_cache.go @@ -229,7 +229,7 @@ func (c *LRU) PutWithTTL(key string, value interface{}, ttl time.Duration) { c.mut.Lock() defer c.mut.Unlock() - _, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil) + _, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil, true) } // Get returns the value associated with the key, optionally @@ -363,8 +363,8 @@ func (c *LRU) tryCached( if !exists { // The entry doesn't exist, clear enough space for it and then add it - if err := c.reserveCapacity(1); err != nil { - return nil, false, nil, err + if !c.reserveCapacity(1) { + return nil, false, nil, ErrCacheFull } entry = c.newEntry(key) @@ -390,7 +390,7 @@ func (c *LRU) cacheLoadComplete( return c.handleCacheLoadErrorWithLock(key, expiresAt, err) } - return c.updateCacheEntryWithLock(key, expiresAt, value, err) + return c.updateCacheEntryWithLock(key, expiresAt, value, err, false) } // handleCacheLoadErrorWithLock handles the results of an error from a cache load. If @@ -402,7 +402,7 @@ func (c *LRU) handleCacheLoadErrorWithLock( // If the loader is telling us to cache this error, do so unconditionally var cachedErr CachedError if errors.As(err, &cachedErr) { - return c.updateCacheEntryWithLock(key, expiresAt, nil, cachedErr.Err) + return c.updateCacheEntryWithLock(key, expiresAt, nil, cachedErr.Err, false) } // If the cache is configured to cache errors by default, do so unless @@ -410,7 +410,7 @@ func (c *LRU) handleCacheLoadErrorWithLock( var uncachedErr UncachedError isUncachedError := errors.As(err, &uncachedErr) if c.cacheErrors && !isUncachedError { - return c.updateCacheEntryWithLock(key, expiresAt, nil, err) + return c.updateCacheEntryWithLock(key, expiresAt, nil, err, false) } // Something happened during load, but we don't want to cache this - remove the entry, @@ -430,10 +430,15 @@ func (c *LRU) handleCacheLoadErrorWithLock( // updateCacheEntryWithLock updates a cache entry with a new value or cached error, // and marks it as the most recently accessed and most recently loaded entry func (c *LRU) updateCacheEntryWithLock( - key string, expiresAt time.Time, value interface{}, err error, + key string, expiresAt time.Time, value interface{}, err error, enforceLimit bool, ) (interface{}, error) { entry := c.entries[key] if entry == nil { + if enforceLimit && !c.reserveCapacity(1) { + // Silently skip adding the new entry if we fail to free up space for it + // (which should never be happening). + return value, err + } entry = &lruCacheEntry{} c.entries[key] = entry } @@ -471,7 +476,7 @@ func (c *LRU) updateCacheEntryWithLock( // reserveCapacity evicts expired and least recently used entries (that aren't loading) // until we have at least enough space for new entries. // NB(mmihic): Must be called with the cache mutex locked. -func (c *LRU) reserveCapacity(n int) error { +func (c *LRU) reserveCapacity(n int) bool { // Unconditionally evict all expired entries. Entries that are expired by // reloading are not in this list, and therefore will not be evicted. oldestElt := c.byLoadTime.Back() @@ -496,10 +501,10 @@ func (c *LRU) reserveCapacity(n int) error { // If we couldn't create enough space, then there are too many entries loading and the cache is simply full if c.maxEntries-len(c.entries) < n { - return ErrCacheFull + return false } - return nil + return true } // load tries to load from the loader. diff --git a/src/x/cache/lru_cache_prop_test.go b/src/x/cache/lru_cache_prop_test.go index 52c79b7abc..6772f53e38 100644 --- a/src/x/cache/lru_cache_prop_test.go +++ b/src/x/cache/lru_cache_prop_test.go @@ -37,10 +37,10 @@ import ( func TestLRUPropertyTest(t *testing.T) { testLRUPropFunc := func(input propTestMultiInput) (bool, error) { - size := 1000 + maxSize := 1000 cacheOpts := &LRUOptions{ - MaxEntries: size, + MaxEntries: maxSize, Now: time.Now, } @@ -98,6 +98,10 @@ func TestLRUPropertyTest(t *testing.T) { t.Logf("found=%d, not_found=%d\n", found.Load(), notFound.Load()) + if actualSize := len(lru.entries); actualSize > maxSize { + return false, fmt.Errorf("cache size %d exceeded MaxSize %d", actualSize, maxSize) + } + return true, nil } diff --git a/src/x/cache/lru_cache_test.go b/src/x/cache/lru_cache_test.go index e9a2debefc..4641ad9f30 100644 --- a/src/x/cache/lru_cache_test.go +++ b/src/x/cache/lru_cache_test.go @@ -24,6 +24,7 @@ import ( "context" "errors" "fmt" + "strconv" "sync" "sync/atomic" "testing" @@ -636,6 +637,19 @@ func TestLRU_TryGetExpired(t *testing.T) { require.False(t, ok) } +func TestLRU_EnforceMaxEntries(t *testing.T) { + var ( + maxEntries = 2 + lru = NewLRU(&LRUOptions{MaxEntries: maxEntries, TTL: time.Second, Now: time.Now}) + ) + + for i := 0; i <= maxEntries; i++ { + lru.Put(strconv.Itoa(i), "foo") + } + + assert.Len(t, lru.entries, maxEntries) +} + var defaultKeys = []string{ "key-0", "key-1", "key-2", "key-3", "key-4", "key-5", "key-6", "key-7", "key-8", "key-9", "key10", }