Repository: allegro/bigcache
Pull request #319 (2022-05-24 09:29:00)

Unify expiry condition in getWithInfo() and onEvict()
Closes: #318 

janisz: @chen-zhuohan it looks like we have race now
chen-zhuohan: it seems be some problem about uint64(out of the uint64 limits).

```go
func (s *cacheShard) isExpired(oldestEntry []byte, currentTimestamp uint64) bool {
	oldestTimestamp := readTimestampFromEntry(oldestEntry)
	fmt.Println(time.Unix(int64(currentTimestamp), 0), time.Unix(int64(oldestTimestamp), 0), currentTimestamp-oldestTimestamp, s.lifeWindow, currentTimestamp-oldestTimestamp > s.lifeWindow)
	return currentTimestamp-oldestTimestamp > s.lifeWindow
}
```

I get this print:
`2022-05-30 16:02:56 +0800 CST 2022-05-30 16:02:57 +0800 CST 18446744073709551615 600 true`

see 18446744073709551615, and the problem will be clean.

I add another check to fix it.
chen-zhuohan: how long will the change merged QAQ

Patch:
diff --git a/shard.go b/shard.go
index 4e63903..efb3031 100644
--- a/shard.go
+++ b/shard.go
@@ -50,12 +50,11 @@ func (s *cacheShard) getWithInfo(key string, hashedKey uint64) (entry []byte, re
 	}
 
 	entry = readEntry(wrappedEntry)
-	oldestTimeStamp := readTimestampFromEntry(wrappedEntry)
-	s.lock.RUnlock()
-	s.hit(hashedKey)
-	if currentTime-oldestTimeStamp >= s.lifeWindow {
+	if s.isExpired(wrappedEntry, currentTime) {
 		resp.EntryStatus = Expired
 	}
+	s.lock.RUnlock()
+	s.hit(hashedKey)
 	return entry, resp, nil
 }
 
@@ -268,17 +267,21 @@ func (s *cacheShard) del(hashedKey uint64) error {
 }
 
 func (s *cacheShard) onEvict(oldestEntry []byte, currentTimestamp uint64, evict func(reason RemoveReason) error) bool {
-	oldestTimestamp := readTimestampFromEntry(oldestEntry)
-	if currentTimestamp < oldestTimestamp {
-		return false
-	}
-	if currentTimestamp-oldestTimestamp > s.lifeWindow {
+	if s.isExpired(oldestEntry, currentTimestamp) {
 		evict(Expired)
 		return true
 	}
 	return false
 }
 
+func (s *cacheShard) isExpired(oldestEntry []byte, currentTimestamp uint64) bool {
+	oldestTimestamp := readTimestampFromEntry(oldestEntry)
+	if currentTimestamp <= oldestTimestamp { // if currentTimestamp < oldestTimestamp, the result will out of uint64 limits;
+		return false
+	}
+	return currentTimestamp-oldestTimestamp > s.lifeWindow
+}
+
 func (s *cacheShard) cleanUp(currentTimestamp uint64) {
 	s.lock.Lock()
 	for {


Test patch:
diff --git a/bigcache_test.go b/bigcache_test.go
index 0dba29f..8371450 100644
--- a/bigcache_test.go
+++ b/bigcache_test.go
@@ -219,12 +219,21 @@ func TestTimingEviction(t *testing.T) {
 		MaxEntrySize:       256,
 	}, &clock)
 
-	// when
 	cache.Set("key", []byte("value"))
-	clock.set(5)
+
+	// when
+	clock.set(1)
 	cache.Set("key2", []byte("value2"))
 	_, err := cache.Get("key")
 
+	// then
+	noError(t, err)
+
+	// when
+	clock.set(5)
+	cache.Set("key2", []byte("value2"))
+	_, err = cache.Get("key")
+
 	// then
 	assertEqual(t, ErrEntryNotFound, err)
 }
@@ -1066,22 +1075,40 @@ func TestBigCache_GetWithInfo(t *testing.T) {
 	value := "100"
 	cache.Set(key, []byte(value))
 
-	// when
-	data, resp, err := cache.GetWithInfo(key)
-
-	// then
-	assertEqual(t, []byte(value), data)
-	noError(t, err)
-	assertEqual(t, Response{}, resp)
-
-	// when
-	clock.set(5)
-	data, resp, err = cache.GetWithInfo(key)
+	for _, tc := range []struct {
+		name     string
+		clock    int64
+		wantData string
+		wantResp Response
+	}{
+		{
+			name:     "zero",
+			clock:    0,
+			wantData: value,
+			wantResp: Response{},
+		},
+		{
+			name:     "Before Expired",
+			clock:    4,
+			wantData: value,
+			wantResp: Response{},
+		},
+		{
+			name:     "Expired",
+			clock:    5,
+			wantData: value,
+			wantResp: Response{},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			clock.set(tc.clock)
+			data, resp, err := cache.GetWithInfo(key)
 
-	// then
-	assertEqual(t, err, nil)
-	assertEqual(t, Response{EntryStatus: Expired}, resp)
-	assertEqual(t, []byte(value), data)
+			assertEqual(t, []byte(tc.wantData), data)
+			noError(t, err)
+			assertEqual(t, tc.wantResp, resp)
+		})
+	}
 }
 
 type mockedLogger struct {