diff --git a/internal/blobtesting/faulty.go b/internal/blobtesting/faulty.go index 369e7060a..620724e42 100644 --- a/internal/blobtesting/faulty.go +++ b/internal/blobtesting/faulty.go @@ -25,13 +25,14 @@ const ( type FaultyStorage struct { base blob.Storage - fault.Set + *fault.Set } // NewFaultyStorage creates new Storage with fault injection. func NewFaultyStorage(base blob.Storage) *FaultyStorage { return &FaultyStorage{ base: base, + Set: fault.NewSet(), } } diff --git a/internal/fault/fault_set.go b/internal/fault/fault_set.go index 5e2d107ea..684c9607c 100644 --- a/internal/fault/fault_set.go +++ b/internal/fault/fault_set.go @@ -16,16 +16,21 @@ type Method int // Set encapsulates a set of faults. type Set struct { - mu sync.Mutex - faults map[Method][]*Fault + mu sync.Locker + // +checklocks:mu + faults map[Method][]*Fault + // +checklocks:mu callCounter map[Method]int } -func (s *Set) ensureInitialized() { - if s.faults == nil { - s.faults = map[Method][]*Fault{} - s.callCounter = map[Method]int{} +func NewSet() *Set { + q := &Set{ + mu: &sync.Mutex{}, + faults: map[Method][]*Fault{}, + callCounter: map[Method]int{}, } + + return q } // AddFault adds a new fault for a given method. @@ -33,8 +38,6 @@ func (s *Set) AddFault(method Method) *Fault { s.mu.Lock() defer s.mu.Unlock() - s.ensureInitialized() - f := New() s.faults[method] = append(s.faults[method], f) @@ -46,8 +49,6 @@ func (s *Set) AddFaults(method Method, faults ...*Fault) { s.mu.Lock() defer s.mu.Unlock() - s.ensureInitialized() - s.faults[method] = append(s.faults[method], faults...) } @@ -73,10 +74,10 @@ func (s *Set) VerifyAllFaultsExercised(t *testing.T) { // GetNextFault returns the error message to return on next fault. func (s *Set) GetNextFault(ctx context.Context, method Method, args ...interface{}) (bool, error) { + // Lock set for map accesses. Call counters will be updated for the fault-set, and the fault for the fault-set method + // will be gotten. s.mu.Lock() - s.ensureInitialized() - s.callCounter[method]++ faults := s.faults[method] @@ -86,31 +87,53 @@ func (s *Set) GetNextFault(ctx context.Context, method Method, args ...interface return false, nil } + // Access the "next" fault. The fault at the end of the queue f := faults[0] + // `fault` comes from `s.faults` so nested locks held at this point. + f.mu.Lock() + + // Count down repeat count in fault. if f.repeatCount > 0 { f.repeatCount-- log(ctx).Debugf("will repeat %v more times the fault for %v %v", f.repeatCount, method, args) } else { + // `repeatCount` == 0. Remove the fault if there are faults remaining in the queue ... if remaining := faults[1:]; len(remaining) > 0 { s.faults[method] = remaining } else { + // ... otherwise delete the map entry for the method delete(s.faults, method) } } + delay := f.sleep + + // Two locks are held, so unlock both before waiting. + f.mu.Unlock() + s.mu.Unlock() - if f.sleep > 0 { - log(ctx).Debugf("sleeping for %v in %v %v", f.sleep, method, args) - time.Sleep(f.sleep) + if delay > 0 { + // sleep for a while + log(ctx).Debugf("sleeping for %v in %v %v", delay, method, args) + time.Sleep(delay) } - if f.callback != nil { - f.callback() + // Re-acquire fault lock to get callback functions. Callbacks will be called without lock. + f.mu.Lock() + + cb := f.callback + errCb := f.errCallback + + f.mu.Unlock() + + // No more references to `f`. Perform callbacks inline. + if cb != nil { + cb() } - if f.errCallback != nil { - err := f.errCallback() + if errCb != nil { + err := errCb() log(ctx).Debugf("returning %v for %v %v", err, method, args) return true, err