[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]>
kubernetes · Dec 10, 2024 · 24370be · 24370be
1 parent 6fe5fd8
commit 24370be
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 15 deletions.
diff --git a/clock/testing/fake_clock.go b/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)
 	}
 

diff --git a/clock/testing/fake_clock_test.go b/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)