Fix LRU cache eviction behavior (#2353)

* Before this PR, LRU cache will only attempt to evict the last access cache item, if this item is pinned, cache is considered full.
* After this PR, LRU cache will attempt to evict the last access & evictable cache item, even if this item is in the middle.
* Update unit test

common/cache/lru.go

@@ -294,20 +294,15 @@ func (c *lru) putInternal(key interface{}, value interface{}, allowUpdate bool)
 		entry.createTime = time.Now().UTC()
 	}
 
-	c.byKey[key] = c.byAccess.PushFront(entry)
-	if len(c.byKey) > c.maxSize {
-		oldest := c.byAccess.Back().Value.(*entryImpl)
-
-		if oldest.refCount > 0 {
-			// Cache is full with pinned elements
-			// revert the insert and return
-			c.deleteInternal(c.byAccess.Front())
-			return nil, ErrCacheFull
-		}
-
-		c.deleteInternal(c.byAccess.Back())
+	if len(c.byKey) >= c.maxSize {
+		c.evictOnceInternal()
+	}
+	if len(c.byKey) >= c.maxSize {
+		return nil, ErrCacheFull
 	}
 
+	element := c.byAccess.PushFront(entry)
+	c.byKey[key] = element
 	return nil, nil
 }
 
@@ -319,6 +314,21 @@ func (c *lru) deleteInternal(element *list.Element) {
 	delete(c.byKey, entry.key)
 }
 
+func (c *lru) evictOnceInternal() {
+	element := c.byAccess.Back()
+	for element != nil {
+		entry := element.Value.(*entryImpl)
+		if entry.refCount == 0 {
+			c.deleteInternal(element)
+			return
+		}
+
+		// entry.refCount > 0
+		// skip, entry still being referenced
+		element = element.Prev()
+	}
+}
+
 func (c *lru) isEntryExpired(entry *entryImpl, currentTime time.Time) bool {
 	return entry.refCount == 0 && !entry.createTime.IsZero() && currentTime.After(entry.createTime.Add(c.ttl))
 }

common/cache/lru_test.go

@@ -236,9 +236,9 @@ func TestRemovedFuncWithTTL_Pin(t *testing.T) {
 	}
 }
 
-func TestRemovedFuncMaxSize_Pin(t *testing.T) {
+func TestRemovedFuncMaxSize_Pin_MidItem(t *testing.T) {
 	ch := make(chan bool)
-	cache := New(1, &Options{
+	cache := New(2, &Options{
 		TTL: time.Millisecond * 50,
 		Pin: true,
 		RemovedFunc: func(i interface{}) {
@@ -250,25 +250,72 @@ func TestRemovedFuncMaxSize_Pin(t *testing.T) {
 
 	_, err := cache.PutIfNotExist("A", t)
 	assert.NoError(t, err)
-	assert.Equal(t, t, cache.Get("A"))
-	cache.Release("A")
 
 	_, err = cache.PutIfNotExist("B", t)
+	assert.NoError(t, err)
+
+	_, err = cache.PutIfNotExist("C", t)
 	assert.Error(t, err)
+
 	assert.Equal(t, t, cache.Get("A"))
-	cache.Release("A")
-	assert.Nil(t, cache.Get("B"))
+	cache.Release("A") // get will also increase the ref count
+	assert.Equal(t, t, cache.Get("B"))
+	cache.Release("B") // get will also increase the ref count
 
-	// since put if not exist also increase the counter
-	cache.Release("A")
+	cache.Release("B") // B's ref count is 0
+	_, err = cache.PutIfNotExist("C", t)
+	assert.NoError(t, err)
+	assert.Equal(t, t, cache.Get("C"))
+	cache.Release("C") // get will also increase the ref count
+
+	cache.Release("A") // A's ref count is 0
+	cache.Release("C") // C's ref count is 0
 
 	time.Sleep(time.Millisecond * 100)
 	assert.Nil(t, cache.Get("A"))
+	assert.Nil(t, cache.Get("B"))
+	assert.Nil(t, cache.Get("C"))
+}
+
+func TestRemovedFuncMaxSize_Pin_LastItem(t *testing.T) {
+	ch := make(chan bool)
+	cache := New(2, &Options{
+		TTL: time.Millisecond * 50,
+		Pin: true,
+		RemovedFunc: func(i interface{}) {
+			_, ok := i.(*testing.T)
+			assert.True(t, ok)
+			ch <- true
+		},
+	})
+
+	_, err := cache.PutIfNotExist("A", t)
+	assert.NoError(t, err)
 
 	_, err = cache.PutIfNotExist("B", t)
 	assert.NoError(t, err)
+
+	_, err = cache.PutIfNotExist("C", t)
+	assert.Error(t, err)
+
+	assert.Equal(t, t, cache.Get("A"))
+	cache.Release("A") // get will also increase the ref count
 	assert.Equal(t, t, cache.Get("B"))
-	cache.Release("B")
+	cache.Release("B") // get will also increase the ref count
+
+	cache.Release("A") // A's ref count is 0
+	_, err = cache.PutIfNotExist("C", t)
+	assert.NoError(t, err)
+	assert.Equal(t, t, cache.Get("C"))
+	cache.Release("C") // get will also increase the ref count
+
+	cache.Release("B") // B's ref count is 0
+	cache.Release("C") // C's ref count is 0
+
+	time.Sleep(time.Millisecond * 100)
+	assert.Nil(t, cache.Get("A"))
+	assert.Nil(t, cache.Get("B"))
+	assert.Nil(t, cache.Get("C"))
 }
 
 func TestIterator(t *testing.T) {