From a32b54a5b89298593f9e8bee2e6546cd230d3f42 Mon Sep 17 00:00:00 2001 From: Carlos Cobo <699969+toqueteos@users.noreply.github.com> Date: Tue, 16 Jul 2024 01:28:24 +0200 Subject: [PATCH 1/3] chore: fix AfterFunc typos in README.md (#2) --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 8d467af..f0703c4 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ For example, with a timer: ```go fired := false -tmr := mClock.Afterfunc(time.Second, func() { +tmr := mClock.AfterFunc(time.Second, func() { fired = true }) mClock.Advance(time.Second) @@ -86,7 +86,7 @@ goroutines, so _do not_ immediately assert the results: ```go fired := false -tmr := mClock.Afterfunc(time.Second, func() { +tmr := mClock.AfterFunc(time.Second, func() { fired = true }) mClock.Advance(time.Second) @@ -105,7 +105,7 @@ fired := false // set a test timeout so we don't wait the default `go test` timeout for a failure ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) -tmr := mClock.Afterfunc(time.Second, func() { +tmr := mClock.AfterFunc(time.Second, func() { fired = true }) From abe75f0264944930a8954168a3711b7ed3bfca5a Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 18 Oct 2024 10:47:14 +0400 Subject: [PATCH 2/3] fix: fix mock TickerFunc to act like real (#7) fixes #5 fixes #6 Fixes the mock TickerFunc to behave like the real one in that it won't run more than one callback at a time and will wait until the callback completes to return from a `Wait()` call. --- mock.go | 10 ++++- mock_test.go | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/mock.go b/mock.go index 5f97e67..ffda2e7 100644 --- a/mock.go +++ b/mock.go @@ -466,6 +466,8 @@ type mockTickerFunc struct { // cond is a condition Locked on the main Mock.mu cond *sync.Cond + // inProgress is true when we are actively calling f + inProgress bool // done is true when the ticker exits done bool // err holds the error when the ticker exits @@ -479,15 +481,18 @@ func (m *mockTickerFunc) next() time.Time { func (m *mockTickerFunc) fire(_ time.Time) { m.mock.mu.Lock() defer m.mock.mu.Unlock() - if m.done { + if m.done || m.inProgress { return } m.nxt = m.nxt.Add(m.d) m.mock.recomputeNextLocked() + m.inProgress = true m.mock.mu.Unlock() err := m.f() m.mock.mu.Lock() + m.inProgress = false + m.cond.Broadcast() // wake up anything waiting for f to finish if err != nil { m.exitLocked(err) } @@ -507,6 +512,9 @@ func (m *mockTickerFunc) waitForCtx() { <-m.ctx.Done() m.mock.mu.Lock() defer m.mock.mu.Unlock() + for m.inProgress { + m.cond.Wait() + } m.exitLocked(m.ctx.Err()) } diff --git a/mock_test.go b/mock_test.go index 421c3db..47316ca 100644 --- a/mock_test.go +++ b/mock_test.go @@ -2,6 +2,7 @@ package quartz_test import ( "context" + "errors" "testing" "time" @@ -210,3 +211,107 @@ func TestPeek(t *testing.T) { t.Fatal("expected Peek() to return false") } } + +// TestTickerFunc_ContextDoneDuringTick tests that TickerFunc.Wait() can't return while the tick +// function callback is in progress. +func TestTickerFunc_ContextDoneDuringTick(t *testing.T) { + t.Parallel() + testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer testCancel() + mClock := quartz.NewMock(t) + + tickStart := make(chan struct{}) + tickDone := make(chan struct{}) + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + tkr := mClock.TickerFunc(ctx, time.Second, func() error { + close(tickStart) + select { + case <-tickDone: + case <-testCtx.Done(): + t.Error("timeout waiting for tickDone") + } + return nil + }) + w := mClock.Advance(time.Second) + select { + case <-tickStart: + // OK + case <-testCtx.Done(): + t.Fatal("timeout waiting for tickStart") + } + waitErr := make(chan error, 1) + go func() { + waitErr <- tkr.Wait() + }() + cancel() + select { + case <-waitErr: + t.Fatal("wait should not return while tick callback in progress") + case <-time.After(time.Millisecond * 100): + // OK + } + close(tickDone) + select { + case err := <-waitErr: + if !errors.Is(err, context.Canceled) { + t.Fatal("expected context.Canceled error") + } + case <-testCtx.Done(): + t.Fatal("timed out waiting for wait to finish") + } + w.MustWait(testCtx) +} + +// TestTickerFunc_LongCallback tests that we don't call the ticker func a second time while the +// first is still executing. +func TestTickerFunc_LongCallback(t *testing.T) { + t.Parallel() + testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer testCancel() + mClock := quartz.NewMock(t) + + expectedErr := errors.New("callback error") + tickStart := make(chan struct{}) + tickDone := make(chan struct{}) + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + tkr := mClock.TickerFunc(ctx, time.Second, func() error { + close(tickStart) + select { + case <-tickDone: + case <-testCtx.Done(): + t.Error("timeout waiting for tickDone") + } + return expectedErr + }) + w := mClock.Advance(time.Second) + select { + case <-tickStart: + // OK + case <-testCtx.Done(): + t.Fatal("timeout waiting for tickStart") + } + // second tick completes immediately, since it doesn't actually call the + // ticker function. + mClock.Advance(time.Second).MustWait(testCtx) + + waitErr := make(chan error, 1) + go func() { + waitErr <- tkr.Wait() + }() + cancel() + close(tickDone) + + select { + case err := <-waitErr: + // we should get the function error, not the context error, since context was canceled while + // we were calling the function, and it returned an error. + if !errors.Is(err, expectedErr) { + t.Fatalf("wrong error: %s", err) + } + case <-testCtx.Done(): + t.Fatal("timed out waiting for wait to finish") + } + w.MustWait(testCtx) +} From 7f10f7fd6925130f6548710df54992e832cfce2d Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 18 Oct 2024 12:33:57 +0400 Subject: [PATCH 3/3] fix: fix swallowing ticks on TickerFunc (#8) Turns out #7 is bugged. When we refused to call the TickerFunc callback, we didn't advance the next event time, so the Mock kept re-scheduling `fire()` and we could never finish processing on the tick time. --- mock.go | 11 +++++++++-- mock_test.go | 10 +++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/mock.go b/mock.go index ffda2e7..cc8e064 100644 --- a/mock.go +++ b/mock.go @@ -480,17 +480,24 @@ func (m *mockTickerFunc) next() time.Time { func (m *mockTickerFunc) fire(_ time.Time) { m.mock.mu.Lock() - defer m.mock.mu.Unlock() - if m.done || m.inProgress { + if m.done { + m.mock.mu.Unlock() return } m.nxt = m.nxt.Add(m.d) m.mock.recomputeNextLocked() + // we need this check to happen after we've computed the next tick, + // otherwise it will be immediately rescheduled. + if m.inProgress { + m.mock.mu.Unlock() + return + } m.inProgress = true m.mock.mu.Unlock() err := m.f() m.mock.mu.Lock() + defer m.mock.mu.Unlock() m.inProgress = false m.cond.Broadcast() // wake up anything waiting for f to finish if err != nil { diff --git a/mock_test.go b/mock_test.go index 47316ca..8099738 100644 --- a/mock_test.go +++ b/mock_test.go @@ -292,9 +292,13 @@ func TestTickerFunc_LongCallback(t *testing.T) { case <-testCtx.Done(): t.Fatal("timeout waiting for tickStart") } - // second tick completes immediately, since it doesn't actually call the - // ticker function. - mClock.Advance(time.Second).MustWait(testCtx) + // additional ticks complete immediately. + elapsed := time.Duration(0) + for elapsed < 5*time.Second { + d, wt := mClock.AdvanceNext() + elapsed += d + wt.MustWait(testCtx) + } waitErr := make(chan error, 1) go func() {