From bde425012912a72d97ba7f11163db3932f6621df Mon Sep 17 00:00:00 2001 From: "Derrick J. Wippler" Date: Tue, 20 Dec 2022 11:35:12 -0600 Subject: [PATCH] Fix LRU to correctly update expire on value replace (#56) --- go.mod | 17 +++++-- go.sum | 43 ++++++++++++----- groupcache.go | 6 +++ groupcache_test.go | 112 ++++++++++++++++++++++++++++++++++++++++----- lru/lru.go | 11 ++++- 5 files changed, 160 insertions(+), 29 deletions(-) diff --git a/go.mod b/go.mod index f35adcb..72812e3 100644 --- a/go.mod +++ b/go.mod @@ -1,10 +1,19 @@ module github.com/mailgun/groupcache/v2 +go 1.19 + require ( - github.com/golang/protobuf v1.3.1 + github.com/golang/protobuf v1.5.2 github.com/segmentio/fasthash v1.0.3 - github.com/sirupsen/logrus v1.6.0 - golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect + github.com/sirupsen/logrus v1.9.0 + github.com/stretchr/testify v1.8.1 ) -go 1.15 +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + golang.org/x/sys v0.0.0-20220919091848-fb04ddd9f9c8 // indirect + golang.org/x/xerrors v0.0.0-20220609144429-65e65417b02f // indirect + google.golang.org/protobuf v1.28.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) diff --git a/go.sum b/go.sum index e6f4bb4..e1d0665 100644 --- a/go.sum +++ b/go.sum @@ -1,18 +1,37 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg= -github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= -github.com/konsorten/go-windows-terminal-sequences v1.0.3 h1:CE8S1cTafDpPvMhIxNJKvHsGVBgn1xWYf1NbHQhywc8= -github.com/konsorten/go-windows-terminal-sequences v1.0.3/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= +github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= +github.com/golang/protobuf v1.5.2 h1:ROPKBNFfQgOUMifHyP+KYbvpjbdoFNs+aK7DXlji0Tw= +github.com/golang/protobuf v1.5.2/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY= +github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= +github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/segmentio/fasthash v1.0.3 h1:EI9+KE1EwvMLBWwjpRDc+fEM+prwxDYbslddQGtrmhM= github.com/segmentio/fasthash v1.0.3/go.mod h1:waKX8l2N8yckOgmSsXJi7x1ZfdKZ4x7KRMzBtS3oedY= -github.com/sirupsen/logrus v1.6.0 h1:UBcNElsrwanuuMsnGSlYmtmgbb23qDR5dG+6X6Oo89I= -github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88= -github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w= -github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= -golang.org/x/sys v0.0.0-20190422165155-953cdadca894 h1:Cz4ceDQGXuKRnVBDTS23GTn/pU5OE2C0WrNTOYK1Uuc= -golang.org/x/sys v0.0.0-20190422165155-953cdadca894/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k= -golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +github.com/sirupsen/logrus v1.9.0 h1:trlNQbNUG3OdDrDil03MCb1H2o9nJ1x4/5LYw7byDE0= +github.com/sirupsen/logrus v1.9.0/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= +github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= +golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220919091848-fb04ddd9f9c8 h1:h+EGohizhe9XlX18rfpa8k8RAc5XyaeamM+0VHRd4lc= +golang.org/x/sys v0.0.0-20220919091848-fb04ddd9f9c8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20220609144429-65e65417b02f h1:uF6paiQQebLeSXkrTqHqz0MXhXXS1KgF41eUdBNvxK0= +golang.org/x/xerrors v0.0.0-20220609144429-65e65417b02f/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8= +google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= +google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= +google.golang.org/protobuf v1.28.1 h1:d0NfwRgPtno5B1Wa6L2DAG+KivqkdutMf1UhdNx175w= +google.golang.org/protobuf v1.28.1/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/groupcache.go b/groupcache.go index 1eff499..3d1ebec 100644 --- a/groupcache.go +++ b/groupcache.go @@ -582,6 +582,11 @@ func (g *Group) CacheStats(which CacheType) CacheStats { } } +// NowFunc returns the current time which is used by the LRU to +// determine if the value has expired. This can be overridden by +// tests to ensure items are evicted when expired. +var NowFunc lru.NowFunc = time.Now + // cache is a wrapper around an *lru.Cache that adds synchronization, // makes values always be ByteView, and counts the size of all keys and // values. @@ -610,6 +615,7 @@ func (c *cache) add(key string, value ByteView) { defer c.mu.Unlock() if c.lru == nil { c.lru = &lru.Cache{ + Now: NowFunc, OnEvicted: func(key lru.Key, value interface{}) { val := value.(ByteView) c.nbytes -= int64(len(key.(string))) + int64(val.Len()) diff --git a/groupcache_test.go b/groupcache_test.go index 5eca5e2..4a667ed 100644 --- a/groupcache_test.go +++ b/groupcache_test.go @@ -30,9 +30,9 @@ import ( "unsafe" "github.com/golang/protobuf/proto" - pb "github.com/mailgun/groupcache/v2/groupcachepb" "github.com/mailgun/groupcache/v2/testpb" + "github.com/stretchr/testify/assert" ) var ( @@ -536,17 +536,105 @@ func TestContextDeadlineOnPeer(t *testing.T) { } } -func TestCacheRemovesBytesOfReplacedValues(t *testing.T) { - c := cache{} +func TestEnsureSizeReportedCorrectly(t *testing.T) { + c := &cache{} - const key = "test" - keyLen := len(key) + // Add the first value + bv1 := ByteView{s: "first", e: time.Now().Add(100 * time.Second)} + c.add("key1", bv1) + v, ok := c.get("key1") - for _, bytes := range []int{100, 1000, 2000} { - c.add(key, ByteView{b: make([]byte, bytes)}) - expected := int64(bytes + keyLen) - if c.nbytes != expected { - t.Fatalf("%s: expected %d was %d", t.Name(), expected, c.nbytes) - } - } + // Should be len("key1" + "first") == 9 + assert.True(t, ok) + assert.True(t, v.Equal(bv1)) + assert.Equal(t, int64(9), c.bytes()) + + // Add a second value + bv2 := ByteView{s: "second", e: time.Now().Add(200 * time.Second)} + + c.add("key2", bv2) + v, ok = c.get("key2") + + // Should be len("key2" + "second") == (10 + 9) == 19 + assert.True(t, ok) + assert.True(t, v.Equal(bv2)) + assert.Equal(t, int64(19), c.bytes()) + + // Replace the first value with a shorter value + bv3 := ByteView{s: "3", e: time.Now().Add(200 * time.Second)} + + c.add("key1", bv3) + v, ok = c.get("key1") + + // len("key1" + "3") == 5 + // len("key2" + "second") == 10 + assert.True(t, ok) + assert.True(t, v.Equal(bv3)) + assert.Equal(t, int64(15), c.bytes()) + + // Replace the second value with a longer value + bv4 := ByteView{s: "this-string-is-28-chars-long", e: time.Now().Add(200 * time.Second)} + + c.add("key2", bv4) + v, ok = c.get("key2") + + // len("key1" + "3") == 5 + // len("key2" + "this-string-is-28-chars-long") == 32 + assert.True(t, ok) + assert.True(t, v.Equal(bv4)) + assert.Equal(t, int64(37), c.bytes()) +} + +func TestEnsureUpdateExpiredValue(t *testing.T) { + c := &cache{} + curTime := time.Now() + + // Override the now function so we control time + NowFunc = func() time.Time { + return curTime + } + + // Expires in 1 second + c.add("key1", ByteView{s: "value1", e: curTime.Add(time.Second)}) + _, ok := c.get("key1") + assert.True(t, ok) + + // Advance 1.1 seconds into the future + curTime = curTime.Add(time.Millisecond * 1100) + + // Value should have expired + _, ok = c.get("key1") + assert.False(t, ok) + + // Add a new key that expires in 1 second + c.add("key2", ByteView{s: "value2", e: curTime.Add(time.Second)}) + _, ok = c.get("key2") + assert.True(t, ok) + + // Advance 0.5 seconds into the future + curTime = curTime.Add(time.Millisecond * 500) + + // Value should still exist + _, ok = c.get("key2") + assert.True(t, ok) + + // Replace the existing key, this should update the expired time + c.add("key2", ByteView{s: "updated value2", e: curTime.Add(time.Second)}) + _, ok = c.get("key2") + assert.True(t, ok) + + // Advance 0.6 seconds into the future, which puts us past the initial + // expired time for key2. + curTime = curTime.Add(time.Millisecond * 600) + + // Should still exist + _, ok = c.get("key2") + assert.True(t, ok) + + // Advance 1.1 seconds into the future + curTime = curTime.Add(time.Millisecond * 1100) + + // Should not exist + _, ok = c.get("key2") + assert.False(t, ok) } diff --git a/lru/lru.go b/lru/lru.go index 549122f..e278234 100644 --- a/lru/lru.go +++ b/lru/lru.go @@ -22,6 +22,8 @@ import ( "time" ) +type NowFunc func() time.Time + // Cache is an LRU cache. It is not safe for concurrent access. type Cache struct { // MaxEntries is the maximum number of cache entries before @@ -32,6 +34,11 @@ type Cache struct { // executed when an entry is purged from the cache. OnEvicted func(key Key, value interface{}) + // Now is the Now() function the cache will use to determine + // the current time which is used to calculate expired values + // Defaults to time.Now() + Now NowFunc + ll *list.List cache map[interface{}]*list.Element } @@ -53,6 +60,7 @@ func New(maxEntries int) *Cache { MaxEntries: maxEntries, ll: list.New(), cache: make(map[interface{}]*list.Element), + Now: time.Now, } } @@ -68,6 +76,7 @@ func (c *Cache) Add(key Key, value interface{}, expire time.Time) { c.OnEvicted(key, eee.value) } c.ll.MoveToFront(ee) + eee.expire = expire eee.value = value return } @@ -86,7 +95,7 @@ func (c *Cache) Get(key Key) (value interface{}, ok bool) { if ele, hit := c.cache[key]; hit { entry := ele.Value.(*entry) // If the entry has expired, remove it from the cache - if !entry.expire.IsZero() && entry.expire.Before(time.Now()) { + if !entry.expire.IsZero() && entry.expire.Before(c.Now()) { c.removeElement(ele) return nil, false }