Fixed clock.fakeTimer.Stop and Reset

Fixed the way Stop identified the fakeClockWaiter to remove, and the
logic for the bool to return.

Fixed Reset to side-effect the waiter in the clock's slice rather than
the original copy.

Added a test func for FakeClock timers.

Fixed Reset too

Improved comments in clock.go to address reviewer comments

Hopefully the updated comments make clear the answers to recent
reviewer comments.

staging/src/k8s.io/apimachinery/pkg/util/clock/clock.go

@@ -80,7 +80,6 @@ type fakeClockWaiter struct {
 	stepInterval  time.Duration
 	skipIfBlocked bool
 	destChan      chan time.Time
-	fired         bool
 }
 
 func NewFakeClock(t time.Time) *FakeClock {
@@ -175,12 +174,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.stepInterval > 0 {
@@ -287,36 +284,50 @@ 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 conditionally stops the timer.  If the timer has neither fired
+// nor been stopped then this call stops the timer and returns true,
+// otherwise this call returns false.  This is like time.Timer::Stop.
 func (f *fakeTimer) Stop() bool {
 	f.fakeClock.lock.Lock()
 	defer f.fakeClock.lock.Unlock()
-
-	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)
+	// The timer has already fired or been stopped, unless it is found
+	// among the clock's waiters.
+	stopped := false
+	oldWaiters := f.fakeClock.waiters
+	newWaiters := make([]fakeClockWaiter, 0, len(oldWaiters))
+	seekChan := f.waiter.destChan
+	for i := range oldWaiters {
+		// Identify the timer's fakeClockWaiter by the identity of the
+		// destination channel, nothing else is necessarily unique and
+		// constant since the timer's creation.
+		if oldWaiters[i].destChan == seekChan {
+			stopped = true
+		} else {
+			newWaiters = append(newWaiters, oldWaiters[i])
 		}
 	}
 
 	f.fakeClock.waiters = newWaiters
 
-	return !f.waiter.fired
+	return stopped
 }
 
-// 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 conditionally updates the firing time of the timer.  If the
+// timer has neither fired nor been stopped then this call resets the
+// timer to the fake clock's "now" + d and returns true, otherwise
+// this call returns false.  This is like time.Timer::Reset.
 func (f *fakeTimer) Reset(d time.Duration) bool {
 	f.fakeClock.lock.Lock()
 	defer f.fakeClock.lock.Unlock()
-
-	active := !f.waiter.fired
-
-	f.waiter.fired = false
-	f.waiter.targetTime = f.fakeClock.time.Add(d)
-
-	return active
+	waiters := f.fakeClock.waiters
+	seekChan := f.waiter.destChan
+	for i := range waiters {
+		if waiters[i].destChan == seekChan {
+			waiters[i].targetTime = f.fakeClock.time.Add(d)
+			return true
+		}
+	}
+	return false
 }
 
 type Ticker interface {

staging/src/k8s.io/apimachinery/pkg/util/clock/clock_test.go

@@ -117,6 +117,108 @@ func TestFakeAfter(t *testing.T) {
 	}
 }
 
+func TestFakeTimer(t *testing.T) {
+	tc := NewFakeClock(time.Now())
+	if tc.HasWaiters() {
+		t.Errorf("unexpected waiter?")
+	}
+	oneSec := tc.NewTimer(time.Second)
+	twoSec := tc.NewTimer(time.Second * 2)
+	treSec := tc.NewTimer(time.Second * 3)
+	if !tc.HasWaiters() {
+		t.Errorf("unexpected lack of waiter?")
+	}
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	tc.Step(999999999 * time.Nanosecond) // t=.999,999,999
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	tc.Step(time.Nanosecond) // t=1
+	select {
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	select {
+	case <-oneSec.C():
+		// Expected!
+	default:
+		t.Errorf("unexpected channel non-read")
+	}
+	tc.Step(time.Nanosecond) // t=1.000,000,001
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	if oneSec.Stop() {
+		t.Errorf("Expected oneSec.Stop() to return false")
+	}
+	if !twoSec.Stop() {
+		t.Errorf("Expected twoSec.Stop() to return true")
+	}
+	tc.Step(time.Second) // t=2.000,000,001
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	if twoSec.Reset(time.Second) {
+		t.Errorf("Expected twoSec.Reset() to return false")
+	}
+	if !treSec.Reset(time.Second) {
+		t.Errorf("Expected treSec.Reset() to return true")
+	}
+	tc.Step(time.Nanosecond * 999999999) // t=3.0
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	case <-treSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	tc.Step(time.Nanosecond) // t=3.000,000,001
+	select {
+	case <-oneSec.C():
+		t.Errorf("unexpected channel read")
+	case <-twoSec.C():
+		t.Errorf("unexpected channel read")
+	default:
+	}
+	select {
+	case <-treSec.C():
+		// Expected!
+	default:
+		t.Errorf("unexpected channel non-read")
+	}
+}
+
 func TestFakeTick(t *testing.T) {
 	tc := NewFakeClock(time.Now())
 	if tc.HasWaiters() {