From 3e898d69210918433fd0cd0aa89392faac61b83a Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Sat, 8 Jun 2019 00:33:35 -0400 Subject: [PATCH] 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. --- .../apimachinery/pkg/util/clock/clock.go | 51 +++++---- .../apimachinery/pkg/util/clock/clock_test.go | 102 ++++++++++++++++++ 2 files changed, 133 insertions(+), 20 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/clock/clock.go b/staging/src/k8s.io/apimachinery/pkg/util/clock/clock.go index 9567f90060d..0d739d961f1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/clock/clock.go +++ b/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 { diff --git a/staging/src/k8s.io/apimachinery/pkg/util/clock/clock_test.go b/staging/src/k8s.io/apimachinery/pkg/util/clock/clock_test.go index 495c2c3c861..9707acec4b8 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/clock/clock_test.go +++ b/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() {