fix: do not call onEvict when adding an existing item

Calling Add, ContainsOrAdd, or PeekOrAdd with a key that is already in the cache should not trigger an eviction. Resolves #141, closes #152.
hashicorp · Aug 22, 2023 · 68d23fc · 68d23fc
1 parent 1e956f5
commit 68d23fc
Show file tree

Hide file tree

Showing 4 changed files with 239 additions and 4 deletions.
diff --git a/expirable/expirable_lru_test.go b/expirable/expirable_lru_test.go
@@ -517,3 +517,47 @@ func getRand(tb testing.TB) int64 {
 	}
 	return out.Int64()
 }
+
+func (c *LRU[K, V]) wantKeys(t *testing.T, want []K) {
+	t.Helper()
+	got := c.Keys()
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("wrong keys got: %v, want: %v ", got, want)
+	}
+}
+
+func TestCache_EvictionSameKey(t *testing.T) {
+	var evictedKeys []int
+
+	cache := NewLRU[int, struct{}](
+		2,
+		func(key int, _ struct{}) {
+			evictedKeys = append(evictedKeys, key)
+		},
+		0)
+
+	if evicted := cache.Add(1, struct{}{}); evicted {
+		t.Error("First 1: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{1})
+
+	if evicted := cache.Add(2, struct{}{}); evicted {
+		t.Error("2: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{1, 2})
+
+	if evicted := cache.Add(1, struct{}{}); evicted {
+		t.Error("Second 1: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{2, 1})
+
+	if evicted := cache.Add(3, struct{}{}); !evicted {
+		t.Error("3: did not get expected eviction")
+	}
+	cache.wantKeys(t, []int{1, 3})
+
+	want := []int{2}
+	if !reflect.DeepEqual(evictedKeys, want) {
+		t.Errorf("evictedKeys got: %v want: %v", evictedKeys, want)
+	}
+}
diff --git a/lru_test.go b/lru_test.go
@@ -4,6 +4,7 @@
 package lru
 
 import (
+	"reflect"
 	"testing"
 )
 
@@ -293,3 +294,150 @@ func TestLRUResize(t *testing.T) {
 		t.Errorf("Cache should have contained 2 elements")
 	}
 }
+
+func (c *Cache[K, V]) wantKeys(t *testing.T, want []K) {
+	t.Helper()
+	got := c.Keys()
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("wrong keys got: %v, want: %v ", got, want)
+	}
+}
+
+func TestCache_EvictionSameKey(t *testing.T) {
+	t.Run("Add", func(t *testing.T) {
+		var evictedKeys []int
+
+		cache, _ := NewWithEvict(
+			2,
+			func(key int, _ struct{}) {
+				evictedKeys = append(evictedKeys, key)
+			})
+
+		if evicted := cache.Add(1, struct{}{}); evicted {
+			t.Error("First 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1})
+
+		if evicted := cache.Add(2, struct{}{}); evicted {
+			t.Error("2: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1, 2})
+
+		if evicted := cache.Add(1, struct{}{}); evicted {
+			t.Error("Second 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{2, 1})
+
+		if evicted := cache.Add(3, struct{}{}); !evicted {
+			t.Error("3: did not get expected eviction")
+		}
+		cache.wantKeys(t, []int{1, 3})
+
+		want := []int{2}
+		if !reflect.DeepEqual(evictedKeys, want) {
+			t.Errorf("evictedKeys got: %v want: %v", evictedKeys, want)
+		}
+	})
+
+	t.Run("ContainsOrAdd", func(t *testing.T) {
+		var evictedKeys []int
+
+		cache, _ := NewWithEvict(
+			2,
+			func(key int, _ struct{}) {
+				evictedKeys = append(evictedKeys, key)
+			})
+
+		contained, evicted := cache.ContainsOrAdd(1, struct{}{})
+		if contained {
+			t.Error("First 1: got unexpected contained")
+		}
+		if evicted {
+			t.Error("First 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1})
+
+		contained, evicted = cache.ContainsOrAdd(2, struct{}{})
+		if contained {
+			t.Error("2: got unexpected contained")
+		}
+		if evicted {
+			t.Error("2: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1, 2})
+
+		contained, evicted = cache.ContainsOrAdd(1, struct{}{})
+		if !contained {
+			t.Error("Second 1: did not get expected contained")
+		}
+		if evicted {
+			t.Error("Second 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1, 2})
+
+		contained, evicted = cache.ContainsOrAdd(3, struct{}{})
+		if contained {
+			t.Error("3: got unexpected contained")
+		}
+		if !evicted {
+			t.Error("3: did not get expected eviction")
+		}
+		cache.wantKeys(t, []int{2, 3})
+
+		want := []int{1}
+		if !reflect.DeepEqual(evictedKeys, want) {
+			t.Errorf("evictedKeys got: %v want: %v", evictedKeys, want)
+		}
+	})
+
+	t.Run("PeekOrAdd", func(t *testing.T) {
+		var evictedKeys []int
+
+		cache, _ := NewWithEvict(
+			2,
+			func(key int, _ struct{}) {
+				evictedKeys = append(evictedKeys, key)
+			})
+
+		_, contained, evicted := cache.PeekOrAdd(1, struct{}{})
+		if contained {
+			t.Error("First 1: got unexpected contained")
+		}
+		if evicted {
+			t.Error("First 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1})
+
+		_, contained, evicted = cache.PeekOrAdd(2, struct{}{})
+		if contained {
+			t.Error("2: got unexpected contained")
+		}
+		if evicted {
+			t.Error("2: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1, 2})
+
+		_, contained, evicted = cache.PeekOrAdd(1, struct{}{})
+		if !contained {
+			t.Error("Second 1: did not get expected contained")
+		}
+		if evicted {
+			t.Error("Second 1: got unexpected eviction")
+		}
+		cache.wantKeys(t, []int{1, 2})
+
+		_, contained, evicted = cache.PeekOrAdd(3, struct{}{})
+		if contained {
+			t.Error("3: got unexpected contained")
+		}
+		if !evicted {
+			t.Error("3: did not get expected eviction")
+		}
+		cache.wantKeys(t, []int{2, 3})
+
+		want := []int{1}
+		if !reflect.DeepEqual(evictedKeys, want) {
+			t.Errorf("evictedKeys got: %v want: %v", evictedKeys, want)
+		}
+	})
+}
diff --git a/simplelru/lru.go b/simplelru/lru.go
@@ -51,9 +51,6 @@ func (c *LRU[K, V]) Add(key K, value V) (evicted bool) {
 	// Check for existing item
 	if ent, ok := c.items[key]; ok {
 		c.evictList.MoveToFront(ent)
-		if c.onEvict != nil {
-			c.onEvict(key, ent.Value)
-		}
 		ent.Value = value
 		return false
 	}

diff --git a/simplelru/lru_test.go b/simplelru/lru_test.go
@@ -3,7 +3,10 @@
 
 package simplelru
 
-import "testing"
+import (
+	"reflect"
+	"testing"
+)
 
 func TestLRU(t *testing.T) {
 	evictCounter := 0
@@ -207,3 +210,46 @@ func TestLRU_Resize(t *testing.T) {
 		t.Errorf("Cache should have contained 2 elements")
 	}
 }
+
+func (c *LRU[K, V]) wantKeys(t *testing.T, want []K) {
+	t.Helper()
+	got := c.Keys()
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("wrong keys got: %v, want: %v ", got, want)
+	}
+}
+
+func TestCache_EvictionSameKey(t *testing.T) {
+	var evictedKeys []int
+
+	cache, _ := NewLRU(
+		2,
+		func(key int, _ struct{}) {
+			evictedKeys = append(evictedKeys, key)
+		})
+
+	if evicted := cache.Add(1, struct{}{}); evicted {
+		t.Error("First 1: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{1})
+
+	if evicted := cache.Add(2, struct{}{}); evicted {
+		t.Error("2: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{1, 2})
+
+	if evicted := cache.Add(1, struct{}{}); evicted {
+		t.Error("Second 1: got unexpected eviction")
+	}
+	cache.wantKeys(t, []int{2, 1})
+
+	if evicted := cache.Add(3, struct{}{}); !evicted {
+		t.Error("3: did not get expected eviction")
+	}
+	cache.wantKeys(t, []int{1, 3})
+
+	want := []int{2}
+	if !reflect.DeepEqual(evictedKeys, want) {
+		t.Errorf("evictedKeys got: %v want: %v", evictedKeys, want)
+	}
+}