From a6de64937a3bf5298d69854d2ddc398407990d48 Mon Sep 17 00:00:00 2001 From: Axel PREL Date: Thu, 27 Jun 2024 12:19:25 +0200 Subject: [PATCH 1/5] add concurrent delete test --- cache_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/cache_test.go b/cache_test.go index 5e2cef2..c2fe02d 100644 --- a/cache_test.go +++ b/cache_test.go @@ -2,7 +2,9 @@ package cache_test import ( "math/rand" + "strconv" "sync" + "sync/atomic" "testing" "time" @@ -121,3 +123,38 @@ func TestCallJanitor(t *testing.T) { t.Errorf("want items is empty but got %d", len(keys)) } } + +func TestConcurrentDelete(t *testing.T) { + c := cache.New[string, int]() + var ( + wg sync.WaitGroup + stop atomic.Bool + timeout = 10 * time.Second + ) + + if testing.Short() { + timeout = 100 * time.Millisecond + } + time.AfterFunc(timeout, func() { + stop.Store(true) + }) + + wg.Add(1) + go func() { + defer wg.Done() + for k := 1; !stop.Load(); k++ { + c.Set(strconv.Itoa(k), k, cache.WithExpiration(0)) + c.Delete(strconv.Itoa(k)) + } + }() + + wg.Add(1) + go func() { + defer wg.Done() + for !stop.Load() { + c.DeleteExpired() + } + }() + + wg.Wait() +} From 064ff830294dba70a4e8a392924158412354a700 Mon Sep 17 00:00:00 2001 From: Axel PREL Date: Thu, 27 Jun 2024 10:59:50 +0200 Subject: [PATCH 2/5] Refactor DeleteExpired for safer behavior - make sure that expManager queue is not empty before popping, which can happen if an explicit delete happens while DeleteExpired releases the lock. - evict() now returns shouldContinue instead of a shouldBreak to simplify the loop. --- cache.go | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/cache.go b/cache.go index e9f6dea..e40a8a3 100644 --- a/cache.go +++ b/cache.go @@ -50,6 +50,8 @@ func (item *Item[K, V]) hasExpiration() bool { return !item.Expiration.IsZero() } +//Print tries to make a readable log of the item + // Expired returns true if the item has expired. func (item *Item[K, V]) Expired() bool { if !item.hasExpiration() { @@ -241,31 +243,26 @@ func (c *Cache[K, V]) GetOrSet(key K, val V, opts ...ItemOption) (actual V, load // DeleteExpired all expired items from the cache. func (c *Cache[K, V]) DeleteExpired() { - c.mu.Lock() - l := c.expManager.len() - c.mu.Unlock() - evict := func() bool { + c.mu.Lock() + defer c.mu.Unlock() + if c.expManager.len() == 0 { + return false + } key := c.expManager.pop() // if is expired, delete it and return nil instead item, ok := c.cache.Get(key) if ok { if item.Expired() { c.cache.Delete(key) - return false + return true } c.expManager.update(key, item.Expiration) } - return true + return false } - for i := 0; i < l; i++ { - c.mu.Lock() - shouldBreak := evict() - c.mu.Unlock() - if shouldBreak { - break - } + for evict() { } } From 836d04893ce2b7dadb2e81bfc1803a077fe5d6ca Mon Sep 17 00:00:00 2001 From: Axel PREL Date: Thu, 27 Jun 2024 11:45:18 +0200 Subject: [PATCH 3/5] DeleteExpired: avoid unnecessary m.Cache.Get() --- cache.go | 14 +++++--------- expiration.go | 5 +++-- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/cache.go b/cache.go index e40a8a3..c7e8b9f 100644 --- a/cache.go +++ b/cache.go @@ -249,16 +249,12 @@ func (c *Cache[K, V]) DeleteExpired() { if c.expManager.len() == 0 { return false } - key := c.expManager.pop() - // if is expired, delete it and return nil instead - item, ok := c.cache.Get(key) - if ok { - if item.Expired() { - c.cache.Delete(key) - return true - } - c.expManager.update(key, item.Expiration) + key, expiration := c.expManager.pop() + if nowFunc().After(expiration) { + c.cache.Delete(key) + return true } + c.expManager.update(key, expiration) return false } diff --git a/expiration.go b/expiration.go index 44ee4ea..ee5d9c2 100644 --- a/expiration.go +++ b/expiration.go @@ -37,11 +37,12 @@ func (m *expirationManager[K]) len() int { return m.queue.Len() } -func (m *expirationManager[K]) pop() K { +func (m *expirationManager[K]) pop() (K, time.Time) { v := heap.Pop(&m.queue) key := v.(*expirationKey[K]).key + exp := v.(*expirationKey[K]).expiration delete(m.mapping, key) - return key + return key, exp } func (m *expirationManager[K]) remove(key K) { From a613de3d8381bd2c4de96cebbf691cc16f3b2b35 Mon Sep 17 00:00:00 2001 From: Axel PREL Date: Thu, 27 Jun 2024 12:29:55 +0200 Subject: [PATCH 4/5] fix #59 --- cache.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cache.go b/cache.go index c7e8b9f..73c6b9c 100644 --- a/cache.go +++ b/cache.go @@ -254,7 +254,9 @@ func (c *Cache[K, V]) DeleteExpired() { c.cache.Delete(key) return true } - c.expManager.update(key, expiration) + if _, ok := c.cache.Get(key); ok { + c.expManager.update(key, expiration) + } return false } From f59f96fb739ded2118e6762a8950a3cdd46b5327 Mon Sep 17 00:00:00 2001 From: Axel PREL Date: Mon, 1 Jul 2024 11:33:23 +0200 Subject: [PATCH 5/5] remove leftover comment --- cache.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cache.go b/cache.go index 73c6b9c..0648807 100644 --- a/cache.go +++ b/cache.go @@ -50,8 +50,6 @@ func (item *Item[K, V]) hasExpiration() bool { return !item.Expiration.IsZero() } -//Print tries to make a readable log of the item - // Expired returns true if the item has expired. func (item *Item[K, V]) Expired() bool { if !item.hasExpiration() {