pkg/cache: fix race in Add() and Evict() (#3013)

* pkg/cache: fix race in Add() and Evict()

This fixes a race in Add() when the shard is at max capacity and the key
being added is already stored.  Previously, the shard would evict a
random value - when all it needed to do was replace an existing value.
There was a race in how Evict() picked which key to remove, which would
cause concurrent calls to Evict() to remove the same key.

Additionally, this commit removes a lot of the lock contention and a
race around Add() and Evict() by changing them to immediately hold the
write lock.  Previously, they would check conditions with the read lock
held and not re-check those conditions once the write lock was acquired
(this is a race).

* pkg/cache: code review comments

* pkg/cache: simplify Add() logic

1 files changed, 24 insertions, 1 deletions

diff --git a/plugin/pkg/cache/cache_test.go b/plugin/pkg/cache/cache_test.go
index 0c56bb9b3..2714967a6 100644
--- a/plugin/pkg/cache/cache_test.go
+++ b/plugin/pkg/cache/cache_test.go
@@ -3,12 +3,23 @@ package cache
 import "testing"
 
 func TestCacheAddAndGet(t *testing.T) {
-	c := New(4)
+	const N = shardSize * 4
+	c := New(N)
 	c.Add(1, 1)
 
 	if _, found := c.Get(1); !found {
 		t.Fatal("Failed to find inserted record")
 	}
+
+	for i := 0; i < N; i++ {
+		c.Add(uint64(i), 1)
+	}
+	for i := 0; i < N; i++ {
+		c.Add(uint64(i), 1)
+		if c.Len() != N {
+			t.Fatal("A item was unnecessarily evicted from the cache")
+		}
+	}
 }
 
 func TestCacheLen(t *testing.T) {
@@ -30,6 +41,18 @@ func TestCacheLen(t *testing.T) {
 	}
 }
 
+func TestCacheSharding(t *testing.T) {
+	c := New(shardSize)
+	for i := 0; i < shardSize*2; i++ {
+		c.Add(uint64(i), 1)
+	}
+	for i, s := range c.shards {
+		if s.Len() == 0 {
+			t.Errorf("Failed to populate shard: %d", i)
+		}
+	}
+}
+
 func BenchmarkCache(b *testing.B) {
 	b.ReportAllocs()