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
This commit is contained in:
Daniel Nephin 2021-04-27 17:01:12 -04:00
parent 768e0a7d03
commit cf8520d85c
3 changed files with 11 additions and 12 deletions

View File

@ -42,7 +42,6 @@ func (c *Client) ServiceNodes(
if err != nil { if err != nil {
return structs.IndexedCheckServiceNodes{}, cache.ResultMeta{}, err return structs.IndexedCheckServiceNodes{}, cache.ResultMeta{}, err
} }
// TODO: can we store non-pointer
return *result.Value.(*structs.IndexedCheckServiceNodes), cache.ResultMeta{Index: result.Index}, err return *result.Value.(*structs.IndexedCheckServiceNodes), cache.ResultMeta{Index: result.Index}, err
} }

View File

@ -79,7 +79,7 @@ func NewMaterializer(deps Deps) *Materializer {
retryWaiter: deps.Waiter, retryWaiter: deps.Waiter,
updateCh: make(chan struct{}), updateCh: make(chan struct{}),
} }
if deps.Waiter == nil { if v.retryWaiter == nil {
v.retryWaiter = &retry.Waiter{ v.retryWaiter = &retry.Waiter{
MinFailures: 1, MinFailures: 1,
// Start backing off with small increments (200-400ms) which will double // Start backing off with small increments (200-400ms) which will double

View File

@ -109,7 +109,7 @@ type Request interface {
// See agent/cache.Cache.Get for complete documentation. // See agent/cache.Cache.Get for complete documentation.
func (s *Store) Get(ctx context.Context, req Request) (Result, error) { func (s *Store) Get(ctx context.Context, req Request) (Result, error) {
info := req.CacheInfo() info := req.CacheInfo()
key, e, err := s.getEntry(req) key, materializer, err := s.readEntry(req)
if err != nil { if err != nil {
return Result{}, err 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) ctx, cancel := context.WithTimeout(ctx, info.Timeout)
defer cancel() 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 // context.DeadlineExceeded is translated to nil to match the behaviour of
// agent/cache.Cache.Get. // agent/cache.Cache.Get.
if err == nil || errors.Is(err, context.DeadlineExceeded) { if err == nil || errors.Is(err, context.DeadlineExceeded) {
@ -139,7 +139,7 @@ func (s *Store) Notify(
updateCh chan<- cache.UpdateEvent, updateCh chan<- cache.UpdateEvent,
) error { ) error {
info := req.CacheInfo() info := req.CacheInfo()
key, e, err := s.getEntry(req) key, materializer, err := s.readEntry(req)
if err != nil { if err != nil {
return err return err
} }
@ -149,7 +149,7 @@ func (s *Store) Notify(
index := info.MinIndex index := info.MinIndex
for { for {
result, err := e.materializer.getFromView(ctx, index) result, err := materializer.getFromView(ctx, index)
switch { switch {
case ctx.Err() != nil: case ctx.Err() != nil:
return return
@ -182,9 +182,9 @@ func (s *Store) Notify(
return nil 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. // 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() info := req.CacheInfo()
key := makeEntryKey(req.Type(), info) key := makeEntryKey(req.Type(), info)
@ -194,12 +194,12 @@ func (s *Store) getEntry(req Request) (string, entry, error) {
if ok { if ok {
e.requests++ e.requests++
s.byKey[key] = e s.byKey[key] = e
return key, e, nil return key, e.materializer, nil
} }
mat, err := req.NewMaterializer() mat, err := req.NewMaterializer()
if err != nil { if err != nil {
return "", e, err return "", nil, err
} }
ctx, cancel := context.WithCancel(context.Background()) ctx, cancel := context.WithCancel(context.Background())
@ -211,11 +211,11 @@ func (s *Store) getEntry(req Request) (string, entry, error) {
requests: 1, requests: 1,
} }
s.byKey[key] = e 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 // 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) { func (s *Store) releaseEntry(key string) {
s.lock.Lock() s.lock.Lock()
defer s.lock.Unlock() defer s.lock.Unlock()