diff --git a/cmd/mesh-controller/conditions_test.go b/cmd/mesh-controller/conditions_test.go index 6e33426d..35bdefe2 100644 --- a/cmd/mesh-controller/conditions_test.go +++ b/cmd/mesh-controller/conditions_test.go @@ -86,7 +86,7 @@ func TestASilenceThroughTheVerbHoldsAndTheConditionStaysOpen(t *testing.T) { func TestUnreadableConditionsAreNotAWellMesh(t *testing.T) { open := aMesh(t) _, store := withConditionsInMemory(t) - store.Fail = errors.New("the bus is away") + store.SetFail(errors.New("the bus is away")) asked, err := theThreeQuestions(t.Context(), open) if err != nil { t.Fatal(err) @@ -98,7 +98,7 @@ func TestUnreadableConditionsAreNotAWellMesh(t *testing.T) { if !strings.HasPrefix(said, "the open conditions could NOT be read") || strings.Contains(said, "no open conditions") { t.Fatalf("%s", said) } - store.Fail = nil + store.SetFail(nil) asked, _ = theThreeQuestions(t.Context(), open) said = printed(t, func() error { return printStatus(asked) }) if asked.well() && !strings.Contains(said, "no open conditions;") { diff --git a/internal/conditions/memory.go b/internal/conditions/memory.go index 1e23ee25..0c3da547 100644 --- a/internal/conditions/memory.go +++ b/internal/conditions/memory.go @@ -16,18 +16,27 @@ type InMemory struct { values map[string]Entry revision uint64 events []Event - // Fail, when set, is what every read and write answers: a store that is away. - Fail error + // fail, when set, is what every read and write answers: a store that is away. It is set only + // through SetFail, under the lock, because the keeper's telling goroutine reads it while a test + // takes the store away. + fail error } // NewInMemory is an empty store. func NewInMemory() *InMemory { return &InMemory{values: map[string]Entry{}} } +// SetFail makes every read and write answer err from now on, or none when err is nil. +func (m *InMemory) SetFail(err error) { + m.mu.Lock() + defer m.mu.Unlock() + m.fail = err +} + func (m *InMemory) Get(_ context.Context, key string) (Entry, bool, error) { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return Entry{}, false, m.Fail + if m.fail != nil { + return Entry{}, false, m.fail } e, ok := m.values[key] return e, ok, nil @@ -36,8 +45,8 @@ func (m *InMemory) Get(_ context.Context, key string) (Entry, bool, error) { func (m *InMemory) Create(_ context.Context, key string, value []byte) error { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return m.Fail + if m.fail != nil { + return m.fail } if _, ok := m.values[key]; ok { return ErrMoved @@ -50,8 +59,8 @@ func (m *InMemory) Create(_ context.Context, key string, value []byte) error { func (m *InMemory) Update(_ context.Context, key string, value []byte, revision uint64) error { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return m.Fail + if m.fail != nil { + return m.fail } if e, ok := m.values[key]; !ok || e.Revision != revision { return ErrMoved @@ -64,8 +73,8 @@ func (m *InMemory) Update(_ context.Context, key string, value []byte, revision func (m *InMemory) Delete(_ context.Context, key string, revision uint64) error { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return m.Fail + if m.fail != nil { + return m.fail } if e, ok := m.values[key]; !ok || e.Revision != revision { return ErrMoved @@ -77,8 +86,8 @@ func (m *InMemory) Delete(_ context.Context, key string, revision uint64) error func (m *InMemory) All(context.Context) (map[string]Entry, error) { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return nil, m.Fail + if m.fail != nil { + return nil, m.fail } out := make(map[string]Entry, len(m.values)) for k, v := range m.values { @@ -90,8 +99,8 @@ func (m *InMemory) All(context.Context) (map[string]Entry, error) { func (m *InMemory) Append(_ context.Context, e Event) error { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return m.Fail + if m.fail != nil { + return m.fail } m.events = append(m.events, e) return nil @@ -100,8 +109,8 @@ func (m *InMemory) Append(_ context.Context, e Event) error { func (m *InMemory) Since(_ context.Context, since time.Time) ([]Event, error) { m.mu.Lock() defer m.mu.Unlock() - if m.Fail != nil { - return nil, m.Fail + if m.fail != nil { + return nil, m.fail } var out []Event for _, e := range m.events { @@ -118,14 +127,22 @@ type Told struct { mu sync.Mutex Events []Event Names []string - Fail error + // fail, when set, is what every publish answers; set only through SetFail, under the lock. + fail error +} + +// SetFail makes every publish answer err from now on, or none when err is nil. +func (t *Told) SetFail(err error) { + t.mu.Lock() + defer t.mu.Unlock() + t.fail = err } func (t *Told) PublishSeatEvent(_ context.Context, seat, event string, body []byte) error { t.mu.Lock() defer t.mu.Unlock() - if t.Fail != nil { - return t.Fail + if t.fail != nil { + return t.fail } if seat != Seat { return errors.New("told under the wrong seat: " + seat) diff --git a/internal/conditions/memory_test.go b/internal/conditions/memory_test.go new file mode 100644 index 00000000..bd80f374 --- /dev/null +++ b/internal/conditions/memory_test.go @@ -0,0 +1,30 @@ +package conditions + +import ( + "errors" + "fmt" + "testing" +) + +// **A test may take the store away while the keeper is still telling.** The keeper keeps every +// transition from a goroutine of its own, so the memory store's failure is read there while a test +// switches it: it is switched under the store's lock, or the race detector fails the suite at random +// (it once failed TestAnUnreadableStoreClearsNothing). A hundred transitions still being kept while +// the failure is switched a hundred times makes the race certain, not rare, when the lock is skipped. +func TestTheStoreIsTakenAwayWhileTheKeeperIsTelling(t *testing.T) { + k, store, told, _ := keeper(t) + ctx := t.Context() + const n = 100 + for i := range n { + if _, err := k.Observe(ctx, silent(fmt.Sprintf("m%d", i))); err != nil { + t.Fatal(err) + } + } + for range n { + store.SetFail(errors.New("the bus is away")) + store.SetFail(nil) + } + told.SetFail(errors.New("no responders")) + told.SetFail(nil) + settled(t, told, n) +} diff --git a/internal/conditions/store_test.go b/internal/conditions/store_test.go index 2d571abe..91d1fbba 100644 --- a/internal/conditions/store_test.go +++ b/internal/conditions/store_test.go @@ -199,15 +199,17 @@ func TestAnUnreadableStoreClearsNothing(t *testing.T) { if _, err := k.Observe(ctx, silent("ace")); err != nil { t.Fatal(err) } - store.Fail = errors.New("the bus is away") + store.SetFail(errors.New("the bus is away")) if err := k.Reconcile(ctx, "S1", nil); err == nil { t.Fatal("reconciled against a store it could not read") } if _, err := k.Open(ctx); err == nil { t.Fatal("an unreadable store answered as read") } - store.Fail = nil + store.SetFail(nil) + store.mu.Lock() store.values["machine.g14.silent"] = Entry{Value: []byte("{not a condition"), Revision: 99} + store.mu.Unlock() if _, err := k.Open(ctx); err == nil || !strings.Contains(err.Error(), "machine.g14.silent") { t.Fatalf("an unreadable condition was left out rather than said: %v", err) } @@ -362,16 +364,15 @@ func TestTheEventShapeIsTheContract(t *testing.T) { // **A transition the bus will not take is offered again**, and said lost only after TellFor. func TestATransitionIsOfferedAgainWhileTheBusIsAway(t *testing.T) { - store, told, c := NewInMemory(), &Told{Fail: errors.New("no responders")}, newClock() + store, told, c := NewInMemory(), &Told{}, newClock() + told.SetFail(errors.New("no responders")) k := NewKeeper(t.Context(), Options{Store: store, History: store, Teller: told, Now: c.now}) defer k.Close(context.Background()) if _, err := k.Observe(t.Context(), silent("ace")); err != nil { t.Fatal(err) } time.Sleep(300 * time.Millisecond) - told.mu.Lock() - told.Fail = nil - told.mu.Unlock() + told.SetFail(nil) said := settled(t, told, 1) if said[0].Key != "machine.ace.silent" || k.Unsaid() != 0 { t.Fatalf("said %+v, unsaid %d", said, k.Unsaid())