From 5fa0dea63a7af434d4250dda8eef9975f2bb4cd3 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Thu, 22 Apr 2021 14:08:09 -0400
Subject: [PATCH] rpcclient/health: fix data race in a test

The idleTTL was being written and read concurrently. Instead move the idleTTL to a struct
field so that when one test patches the TTL it does not impact others.

The background goroutines for the store can outlive a test because context cancellation
is async.
---
 agent/submatview/store.go      | 14 ++++++++------
 agent/submatview/store_test.go | 13 ++-----------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/agent/submatview/store.go b/agent/submatview/store.go
index 18fa7b2bb0..ff684b48d0 100644
--- a/agent/submatview/store.go
+++ b/agent/submatview/store.go
@@ -17,6 +17,11 @@ type Store struct {
 	lock       sync.RWMutex
 	byKey      map[string]entry
 	expiryHeap *ttlcache.ExpiryHeap
+
+	// idleTTL is the duration of time an entry should remain in the Store after the
+	// last request for that entry has been terminated. It is a field on the struct
+	// so that it can be patched in tests without need a lock.
+	idleTTL time.Duration
 }
 
 type entry struct {
@@ -33,6 +38,7 @@ func NewStore(logger hclog.Logger) *Store {
 		logger:     logger,
 		byKey:      make(map[string]entry),
 		expiryHeap: ttlcache.NewExpiryHeap(),
+		idleTTL:    20 * time.Minute,
 	}
 }
 
@@ -186,10 +192,6 @@ func (s *Store) getEntry(req Request) (string, entry, error) {
 	return key, e, nil
 }
 
-// idleTTL is the duration of time an entry should remain in the Store after the
-// last request for that entry has been terminated.
-var idleTTL = 20 * time.Minute
-
 // releaseEntry decrements the request count and starts an expiry timer if the
 // count has reached 0. Must be called once for every call to getEntry.
 func (s *Store) releaseEntry(key string) {
@@ -204,12 +206,12 @@ func (s *Store) releaseEntry(key string) {
 	}
 
 	if e.expiry.Index() == ttlcache.NotIndexed {
-		e.expiry = s.expiryHeap.Add(key, idleTTL)
+		e.expiry = s.expiryHeap.Add(key, s.idleTTL)
 		s.byKey[key] = e
 		return
 	}
 
-	s.expiryHeap.Update(e.expiry.Index(), idleTTL)
+	s.expiryHeap.Update(e.expiry.Index(), s.idleTTL)
 }
 
 // makeEntryKey matches agent/cache.makeEntryKey, but may change in the future.
diff --git a/agent/submatview/store_test.go b/agent/submatview/store_test.go
index 8d6105036c..4a0699bae4 100644
--- a/agent/submatview/store_test.go
+++ b/agent/submatview/store_test.go
@@ -391,10 +391,9 @@ func TestStore_Run_ExpiresEntries(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	ttl := 10 * time.Millisecond
-	patchIdleTTL(t, ttl)
-
 	store := NewStore(hclog.New(nil))
+	ttl := 10 * time.Millisecond
+	store.idleTTL = ttl
 	go store.Run(ctx)
 
 	req := &fakeRequest{
@@ -430,14 +429,6 @@ func TestStore_Run_ExpiresEntries(t *testing.T) {
 	require.Equal(t, ttlcache.NotIndexed, e.expiry.Index())
 }
 
-func patchIdleTTL(t *testing.T, ttl time.Duration) {
-	orig := idleTTL
-	idleTTL = ttl
-	t.Cleanup(func() {
-		idleTTL = orig
-	})
-}
-
 func runStep(t *testing.T, name string, fn func(t *testing.T)) {
 	t.Helper()
 	if !t.Run(name, fn) {