From cf8520d85c89a173e6b51dddf7c21fa22195e11a Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Tue, 27 Apr 2021 17:01:12 -0400
Subject: [PATCH] submatview: only return materializer from getEntry

Also rename it to readEntry now that it doesn't return the entire entry. Based on feedback
in PR review, the full entry is not used by the caller, and accessing the fields wouldn't be
safe outside the lock, so it is safer to return only the Materializer
---
 agent/rpcclient/health/health.go |  1 -
 agent/submatview/materializer.go |  2 +-
 agent/submatview/store.go        | 20 ++++++++++----------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/agent/rpcclient/health/health.go b/agent/rpcclient/health/health.go
index 375a5078d..b8b023e77 100644
--- a/agent/rpcclient/health/health.go
+++ b/agent/rpcclient/health/health.go
@@ -42,7 +42,6 @@ func (c *Client) ServiceNodes(
 		if err != nil {
 			return structs.IndexedCheckServiceNodes{}, cache.ResultMeta{}, err
 		}
-		// TODO: can we store non-pointer
 		return *result.Value.(*structs.IndexedCheckServiceNodes), cache.ResultMeta{Index: result.Index}, err
 	}
 
diff --git a/agent/submatview/materializer.go b/agent/submatview/materializer.go
index 3e3381e74..413b5d3f1 100644
--- a/agent/submatview/materializer.go
+++ b/agent/submatview/materializer.go
@@ -79,7 +79,7 @@ func NewMaterializer(deps Deps) *Materializer {
 		retryWaiter: deps.Waiter,
 		updateCh:    make(chan struct{}),
 	}
-	if deps.Waiter == nil {
+	if v.retryWaiter == nil {
 		v.retryWaiter = &retry.Waiter{
 			MinFailures: 1,
 			// Start backing off with small increments (200-400ms) which will double
diff --git a/agent/submatview/store.go b/agent/submatview/store.go
index fd0179780..aaf083d94 100644
--- a/agent/submatview/store.go
+++ b/agent/submatview/store.go
@@ -109,7 +109,7 @@ type Request interface {
 // See agent/cache.Cache.Get for complete documentation.
 func (s *Store) Get(ctx context.Context, req Request) (Result, error) {
 	info := req.CacheInfo()
-	key, e, err := s.getEntry(req)
+	key, materializer, err := s.readEntry(req)
 	if err != nil {
 		return Result{}, err
 	}
@@ -118,7 +118,7 @@ func (s *Store) Get(ctx context.Context, req Request) (Result, error) {
 	ctx, cancel := context.WithTimeout(ctx, info.Timeout)
 	defer cancel()
 
-	result, err := e.materializer.getFromView(ctx, info.MinIndex)
+	result, err := materializer.getFromView(ctx, info.MinIndex)
 	// context.DeadlineExceeded is translated to nil to match the behaviour of
 	// agent/cache.Cache.Get.
 	if err == nil || errors.Is(err, context.DeadlineExceeded) {
@@ -139,7 +139,7 @@ func (s *Store) Notify(
 	updateCh chan<- cache.UpdateEvent,
 ) error {
 	info := req.CacheInfo()
-	key, e, err := s.getEntry(req)
+	key, materializer, err := s.readEntry(req)
 	if err != nil {
 		return err
 	}
@@ -149,7 +149,7 @@ func (s *Store) Notify(
 
 		index := info.MinIndex
 		for {
-			result, err := e.materializer.getFromView(ctx, index)
+			result, err := materializer.getFromView(ctx, index)
 			switch {
 			case ctx.Err() != nil:
 				return
@@ -182,9 +182,9 @@ func (s *Store) Notify(
 	return nil
 }
 
-// getEntry from the store, and increment the requests counter. releaseEntry
+// readEntry from the store, and increment the requests counter. releaseEntry
 // must be called when the request is finished to decrement the counter.
-func (s *Store) getEntry(req Request) (string, entry, error) {
+func (s *Store) readEntry(req Request) (string, *Materializer, error) {
 	info := req.CacheInfo()
 	key := makeEntryKey(req.Type(), info)
 
@@ -194,12 +194,12 @@ func (s *Store) getEntry(req Request) (string, entry, error) {
 	if ok {
 		e.requests++
 		s.byKey[key] = e
-		return key, e, nil
+		return key, e.materializer, nil
 	}
 
 	mat, err := req.NewMaterializer()
 	if err != nil {
-		return "", e, err
+		return "", nil, err
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())
@@ -211,11 +211,11 @@ func (s *Store) getEntry(req Request) (string, entry, error) {
 		requests:     1,
 	}
 	s.byKey[key] = e
-	return key, e, nil
+	return key, e.materializer, nil
 }
 
 // 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.
+// count has reached 0. Must be called once for every call to readEntry.
 func (s *Store) releaseEntry(key string) {
 	s.lock.Lock()
 	defer s.lock.Unlock()