[Fake clock] Make Stop / Reset return false if Timer stopped (#320)

* [Fake clock] Make Stop / Reset return false if Timer stopped

In order to conform to the standard library Timer, Stop and Reset should
return false if the Timer has already been stopped, which was not the
case previously.

We add unit tests to validate the new implementation. The updated
TestFakeStop test case and the new TestFakeReset/reset_stopped_timer
case fail with the old implementation.

We also simplify the implementation of Stop and Rest in the following
way: there is no need to check f.waiter.fired to determine whether a
Timer has expired. For Timers, this flag is set atomically with the
waiter being removed from the waiters list. As a result, we only check
for absence of the waiter from the list, which either indicates that the
Timer has been stopped or that it has already expired (fired).

Signed-off-by: Antonin Bas <[email protected]>

* Address review comments

Removed unused fired field for waiters.

Signed-off-by: Antonin Bas <[email protected]>

---------

Signed-off-by: Antonin Bas <[email protected]>

Author: antoninbas

clock/testing/fake_clock.go

@@ -48,7 +48,6 @@ type fakeClockWaiter struct {
 	stepInterval  time.Duration
 	skipIfBlocked bool
 	destChan      chan time.Time
-	fired         bool
 	afterFunc     func()
 }
 
@@ -198,12 +197,10 @@ func (f *FakeClock) setTimeLocked(t time.Time) {
 			if w.skipIfBlocked {
 				select {
 				case w.destChan <- t:
-					w.fired = true
 				default:
 				}
 			} else {
 				w.destChan <- t
-				w.fired = true
 			}
 
 			if w.afterFunc != nil {
@@ -305,44 +302,48 @@ func (f *fakeTimer) C() <-chan time.Time {
 	return f.waiter.destChan
 }
 
-// Stop stops the timer and returns true if the timer has not yet fired, or false otherwise.
+// Stop prevents the Timer from firing. It returns true if the call stops the
+// timer, false if the timer has already expired or been stopped.
 func (f *fakeTimer) Stop() bool {
 	f.fakeClock.lock.Lock()
 	defer f.fakeClock.lock.Unlock()
 
+	active := false
 	newWaiters := make([]*fakeClockWaiter, 0, len(f.fakeClock.waiters))
 	for i := range f.fakeClock.waiters {
 		w := f.fakeClock.waiters[i]
 		if w != &f.waiter {
 			newWaiters = append(newWaiters, w)
+			continue
 		}
+		// If timer is found, it has not been fired yet.
+		active = true
 	}
 
 	f.fakeClock.waiters = newWaiters
 
-	return !f.waiter.fired
+	return active
 }
 
-// Reset resets the timer to the fake clock's "now" + d. It returns true if the timer has not yet
-// fired, or false otherwise.
+// Reset changes the timer to expire after duration d. It returns true if the
+// timer had been active, false if the timer had expired or been stopped.
 func (f *fakeTimer) Reset(d time.Duration) bool {
 	f.fakeClock.lock.Lock()
 	defer f.fakeClock.lock.Unlock()
 
-	active := !f.waiter.fired
+	active := false
 
-	f.waiter.fired = false
 	f.waiter.targetTime = f.fakeClock.time.Add(d)
 
-	var isWaiting bool
 	for i := range f.fakeClock.waiters {
 		w := f.fakeClock.waiters[i]
 		if w == &f.waiter {
-			isWaiting = true
+			// If timer is found, it has not been fired yet.
+			active = true
 			break
 		}
 	}
-	if !isWaiting {
+	if !active {
 		f.fakeClock.waiters = append(f.fakeClock.waiters, &f.waiter)
 	}
 

clock/testing/fake_clock_test.go

@@ -281,14 +281,19 @@ func TestFakeStop(t *testing.T) {
 	if !tc.HasWaiters() {
 		t.Errorf("expected a waiter to be present, but it is not")
 	}
-	timer.Stop()
+	if !timer.Stop() {
+		t.Errorf("stop should return true as we are stopping an unexpired timer")
+	}
 	if tc.HasWaiters() {
 		t.Errorf("expected existing waiter to be cleaned up, but it is still present")
 	}
+	if timer.Stop() {
+		t.Errorf("stop should return false as the timer has already been stopped")
+	}
 }
 
 // This tests the pattern documented in the go docs here: https://golang.org/pkg/time/#Timer.Stop
-// This pattern is required to safely reset a timer, so should be common.
+// This pattern is required to safely reset a timer prior to Go 1.23, so should be common.
 // This also tests resetting the timer
 func TestFakeStopDrain(t *testing.T) {
 	start := time.Time{}
@@ -303,7 +308,9 @@ func TestFakeStopDrain(t *testing.T) {
 		t.Errorf("timer should have ticked after 1 second, got %v", readTime)
 	}
 
-	timer.Reset(time.Second)
+	if timer.Reset(time.Second) {
+		t.Errorf("reset should return false as the timer had expired")
+	}
 	if !tc.HasWaiters() {
 		t.Errorf("expected a waiter to be present, but it is not")
 	}
@@ -318,6 +325,79 @@ func TestFakeStopDrain(t *testing.T) {
 	}
 }
 
+func TestFakeReset(t *testing.T) {
+	start := time.Now()
+	t.Run("reset active timer", func(t *testing.T) {
+		tc := NewFakeClock(start)
+		timer := tc.NewTimer(time.Second)
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		tc.Step(999 * time.Millisecond)
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		if !timer.Reset(time.Second) {
+			t.Errorf("reset should return true as the timer is active")
+		}
+		tc.Step(time.Millisecond)
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		tc.Step(999 * time.Millisecond)
+		if tc.HasWaiters() {
+			t.Errorf("expected existing waiter to be cleaned up, but it is still present")
+		}
+		if readTime := assertReadTime(t, timer.C()); !readTime.Equal(start.Add(1999 * time.Millisecond)) {
+			t.Errorf("timer should have ticked after reset + 1 second, got %v", readTime)
+		}
+	})
+
+	t.Run("reset expired timer", func(t *testing.T) {
+		tc := NewFakeClock(start)
+		timer := tc.NewTimer(time.Second)
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		tc.Step(time.Second)
+		if tc.HasWaiters() {
+			t.Errorf("expected existing waiter to be cleaned up, but it is still present")
+		}
+		if readTime := assertReadTime(t, timer.C()); !readTime.Equal(start.Add(time.Second)) {
+			t.Errorf("timer should have ticked after 1 second, got %v", readTime)
+		}
+		if timer.Reset(time.Second) {
+			t.Errorf("reset should return false as the timer had expired")
+		}
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		tc.Step(time.Second)
+		if readTime := assertReadTime(t, timer.C()); !readTime.Equal(start.Add(2 * time.Second)) {
+			t.Errorf("timer should have ticked again after reset + 1 more second, got %v", readTime)
+		}
+	})
+
+	t.Run("reset stopped timer", func(t *testing.T) {
+		tc := NewFakeClock(start)
+		timer := tc.NewTimer(time.Second)
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		timer.Stop()
+		if timer.Reset(time.Second) {
+			t.Errorf("reset should return false as the timer had been stopped")
+		}
+		if !tc.HasWaiters() {
+			t.Errorf("expected a waiter to be present, but it is not")
+		}
+		tc.Step(time.Second)
+		if readTime := assertReadTime(t, timer.C()); !readTime.Equal(start.Add(time.Second)) {
+			t.Errorf("timer should have ticked after reset + 1 second, got %v", readTime)
+		}
+	})
+}
+
 func TestTimerNegative(t *testing.T) {
 	tc := NewFakeClock(time.Now())
 	timer := tc.NewTimer(-1 * time.Second)