Fixing data race (#29)

When reading and writing the same key simultaneously, there were two races that could occur while escalating the lock from a reader lock to a writer lock in the `get` method.

* The expiration check in `get` was reading `pqi.expiration`
* The returned value in `get` was reading `pqi.value`

Both of happened after the call to `lru.lock.RUnlock()`, which means that set could have modified the item that `pqi` was pointing to. We are now copying the underlying item (`*pqi`) to the stack and referencing that. This is a pessimistic way to approach this problem, but it is safe.

A test was added that highlighted these races when run with `-race`.

Thanks @coxley for pointing these out and giving me a test that showed the problem.

go.mod

@@ -2,9 +2,15 @@ module github.com/TriggerMail/lazylru
 
 go 1.20
 
-require github.com/stretchr/testify v1.9.0
+require (
+	github.com/stretchr/testify v1.9.0
+	golang.org/x/sync v0.7.0
+)
 
-require github.com/klauspost/cpuid/v2 v2.0.9 // indirect
+require (
+	github.com/klauspost/cpuid/v2 v2.2.8 // indirect
+	golang.org/x/sys v0.21.0 // indirect
+)
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect

go.sum

@@ -2,13 +2,20 @@ 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/klauspost/cpuid/v2 v2.0.9 h1:lgaqFMSdTdQYdZ04uHyN2d/eKdOMyi2YLSvlQIBFYa4=
 github.com/klauspost/cpuid/v2 v2.0.9/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg=
+github.com/klauspost/cpuid/v2 v2.2.8 h1:+StwCXwm9PdpiEkPyzBXIy+M9KUb4ODm0Zarf1kS5BM=
+github.com/klauspost/cpuid/v2 v2.2.8/go.mod h1:Lcz8mBdAVJIBVzewtcLocK12l3Y+JytZYpaMropDUws=
 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/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
 github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
 github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0=
 github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA=
+golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
+golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
+golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
+golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 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.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

lazylru.go

@@ -256,21 +256,28 @@ func (lru *LazyLRU[K, V]) shouldBubble(index int) bool {
 // key was found in the cache.
 func (lru *LazyLRU[K, V]) Get(key K) (V, bool) {
 	lru.lock.RLock()
+	// pqi may be touched between when we release this lock and the writer lock
+	// below, so we need to store the value we read in the stack before checking
+	// the expiration and such. It won't hurt anything because we will take a
+	// write lock and check pqi again, but it's the right thing to do and makes
+	// the race detector happy.
 	pqi, ok := lru.index[key]
-	lru.lock.RUnlock()
 	if !ok {
+		lru.lock.RUnlock()
 		atomic.AddUint32(&lru.stats.KeysReadNotFound, 1)
 		var zero V
 		return zero, false
 	}
+	qi := *pqi
+	lru.lock.RUnlock()
 
 	// there is a dangerous case if the read/lock/read pattern returns an
 	// unexpired key on the second read -- if we are not careful, we may end up
 	// trying to take the lock twice. Because "defer" can't help us here, I'm
 	// being really explicit about whether or not we have the lock already.
-	locked := false
+	var locked bool
 	// if the item is expired, remove it
-	if pqi.expiration.Before(time.Now()) && pqi.index >= 0 {
+	if qi.expiration.Before(time.Now()) && qi.index >= 0 {
 		lru.lock.Lock()
 		locked = true
 
@@ -308,7 +315,7 @@ func (lru *LazyLRU[K, V]) Get(key K) (V, bool) {
 		lru.lock.Unlock()
 	}
 	atomic.AddUint32(&lru.stats.KeysReadOK, 1)
-	return pqi.value, ok
+	return qi.value, ok
 }
 
 // MGet retrieves values from the cache. Missing values will not be returned.

lazylru_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/sync/errgroup"
+
 	lazylru "github.com/TriggerMail/lazylru"
 	"github.com/stretchr/testify/require"
 )
@@ -509,3 +511,24 @@ func TestCallbackOnExpire(t *testing.T) {
 	time.Sleep(100 * time.Millisecond)
 	require.Equal(t, 5, len(evicted), "on evict items")
 }
+
+func TestConcurrent(t *testing.T) {
+	lru := lazylru.NewT[int, int](2000, time.Hour)
+
+	var group errgroup.Group
+	group.Go(func() error {
+		for n := 0; n < 1000; n++ {
+			lru.Set(0, 0)
+		}
+		return nil
+	})
+
+	group.Go(func() error {
+		for n := 0; n < 1000; n++ {
+			lru.Get(0)
+		}
+		return nil
+	})
+
+	_ = group.Wait()
+}