refactor: implement do method for timer

This change adds a do method for timer (similar to ticker’s do method).
It also adds a test case to verifies that backwards compatibility is
preserved for a quirk in the current Observe and ticker implementation.

That is, a tick triggers the observeUnlocked method. I’m not aware of
any piece of code actually relying on this behavior, but it’s better to
change that separately if we want to.

Additionally, we introduce a small convention for Time’s methods: if the
method does not acquire the lock, it has Unlocked suffix. A notable
exception is the do method on timer and ticker that is expected to run
under Time’s lock when scheduled temporal effects are triggered.

ticker.go

@@ -16,11 +16,11 @@ func (t *ticker) C() <-chan time.Time {
 }
 
 func (t *ticker) Stop() {
-	t.time.stopTimer(t.id)
+	t.time.stop(t.id)
 }
 
 func (t *ticker) Reset(d time.Duration) {
-	t.time.resetTicker(t, d)
+	t.time.reset(d, t.id, t.do, &t.dur)
 }
 
 // do is the ticker’s moment callback. It sends the now time to the underlying
@@ -29,8 +29,15 @@ func (t *ticker) Reset(d time.Duration) {
 func (t *ticker) do(now time.Time) {
 	t.ch <- now
 
-	// It is safe to mutate ID without a lock since at most one
-	// moment exists for the given ticker and moments run under
-	// the Time’s lock.
+	// It is safe to mutate ID without a lock since at most one moment
+	// exists for the given ticker and moments run under the Time’s lock.
+	//
+	// Additionally, while we probably should be resetting the moment with
+	// the initial ticker’s ID, it is not possible since that would break
+	// backwards compatibility for users that rely on Time’s Observe method
+	// to observe ticks.
+	//
+	//  t.time.resetUnlocked(t.dur, t.id, t.do, nil)
+	//
 	t.id = t.time.planUnlocked(now.Add(t.dur), t.do)
 }

time.go

@@ -32,6 +32,8 @@ func NewTime(now time.Time) *Time {
 //
 // All methods are goroutine-safe.
 type Time struct {
+	// mux guards internal state. Note that all methods without Unlocked
+	// suffix acquire mux.
 	mux      sync.Mutex
 	now      time.Time
 	momentID int
@@ -41,25 +43,22 @@ type Time struct {
 }
 
 func (t *Time) Timer(d time.Duration) Timer {
-	done := make(chan time.Time, 1)
-
-	return timer{
+	tt := &timer{
 		time: t,
-		ch:   done,
-		id: t.plan(t.When(d), func(now time.Time) {
-			done <- now
-		}),
+		ch:   make(chan time.Time, 1),
 	}
+	tt.id = t.plan(t.When(d), tt.do)
+	return tt
 }
 
 func (t *Time) Ticker(d time.Duration) Ticker {
-	tick := &ticker{
+	tt := &ticker{
 		time: t,
 		ch:   make(chan time.Time, 1),
 		dur:  d,
 	}
-	tick.id = t.plan(t.When(d), tick.do)
-	return tick
+	tt.id = t.plan(t.When(d), tt.do)
+	return tt
 }
 
 func (t *Time) planUnlocked(when time.Time, do func(now time.Time)) int {
@@ -69,7 +68,7 @@ func (t *Time) planUnlocked(when time.Time, do func(now time.Time)) int {
 		when: when,
 		do:   do,
 	}
-	t.observe()
+	t.observeUnlocked()
 	return id
 }
 
@@ -80,7 +79,9 @@ func (t *Time) plan(when time.Time, do func(now time.Time)) int {
 	return t.planUnlocked(when, do)
 }
 
-func (t *Time) stopTimer(id int) bool {
+// stop removes the moment with the given ID from the list of scheduled moments.
+// It returns true if a moment existed for the given ID, otherwise it is no-op.
+func (t *Time) stop(id int) bool {
 	t.mux.Lock()
 	defer t.mux.Unlock()
 
@@ -89,44 +90,32 @@ func (t *Time) stopTimer(id int) bool {
 	return ok
 }
 
-func (t *Time) resetTimer(d time.Duration, id int, ch chan time.Time) {
+// reset adjusts the moment with the given ID to run after the d duration. It
+// creates a new moment if the moment does not already exist. If durp pointer
+// is not nil, it is updated with d value while reset is holding Time’s lock.
+func (t *Time) reset(d time.Duration, id int, do func(now time.Time), durp *time.Duration) {
 	t.mux.Lock()
 	defer t.mux.Unlock()
-
-	m, ok := t.moments[id]
-	if !ok {
-		m = moment{
-			do: func(now time.Time) {
-				ch <- now
-			},
-		}
-	}
-
-	m.when = t.now.Add(d)
-	t.moments[id] = m
+	t.resetUnlocked(d, id, do, durp)
 }
 
-// resetTicker resets the moment of the given ticker to run after the duration d.
-func (t *Time) resetTicker(tick *ticker, d time.Duration) {
-	t.mux.Lock()
-	defer t.mux.Unlock()
-
-	tick.dur = d
-	id := tick.id
+// resetUnlocked is like reset but does not acquire the Time’s lock.
+func (t *Time) resetUnlocked(d time.Duration, id int, do func(now time.Time), durp *time.Duration) {
+	if durp != nil {
+		*durp = d
+	}
 
 	m, ok := t.moments[id]
 	if !ok {
-		m = moment{do: tick.do}
+		m = moment{do: do}
 	}
 
 	m.when = t.now.Add(d)
 	t.moments[id] = m
 }
 
-// tick applies all scheduled temporal effects.
-//
-// The mux lock is expected.
-func (t *Time) tick() moments {
+// tickUnlocked applies all scheduled temporal effects.
+func (t *Time) tickUnlocked() moments {
 	var past moments
 
 	for id, m := range t.moments {
@@ -144,30 +133,27 @@ func (t *Time) tick() moments {
 // Now returns the current time.
 func (t *Time) Now() time.Time {
 	t.mux.Lock()
-	now := t.now
-	t.mux.Unlock()
-	return now
+	defer t.mux.Unlock()
+	return t.now
 }
 
 // Set travels to specified time.
 //
 // Also triggers temporal effects.
 func (t *Time) Set(now time.Time) {
 	t.mux.Lock()
-	t.now = now
-	t.mux.Unlock()
-	t.tick().do(now)
+	defer t.mux.Unlock()
+	t.setUnlocked(now)
 }
 
 // Travel adds duration to current time and returns result.
 //
 // Also triggers temporal effects.
 func (t *Time) Travel(d time.Duration) time.Time {
 	t.mux.Lock()
+	defer t.mux.Unlock()
 	now := t.now.Add(d)
-	t.now = now
-	t.tick().do(now)
-	t.mux.Unlock()
+	t.setUnlocked(now)
 	return now
 }
 
@@ -176,13 +162,19 @@ func (t *Time) Travel(d time.Duration) time.Time {
 // Also triggers temporal effects.
 func (t *Time) TravelDate(years, months, days int) time.Time {
 	t.mux.Lock()
+	defer t.mux.Unlock()
 	now := t.now.AddDate(years, months, days)
-	t.now = now
-	t.tick().do(now)
-	t.mux.Unlock()
+	t.setUnlocked(now)
 	return now
 }
 
+// setUnlocked sets the current time to the given now time and triggers temporal
+// effects.
+func (t *Time) setUnlocked(now time.Time) {
+	t.now = now
+	t.tickUnlocked().do(now)
+}
+
 // Sleep blocks until duration is elapsed.
 func (t *Time) Sleep(d time.Duration) { <-t.After(d) }
 
@@ -201,7 +193,8 @@ func (t *Time) After(d time.Duration) <-chan time.Time {
 	return done
 }
 
-// Observe return channel that closes on clock calls.
+// Observe return channel that closes on clock calls. The current implementation
+// also closes the channel on Ticker’s ticks.
 func (t *Time) Observe() <-chan struct{} {
 	observer := make(chan struct{})
 	t.mux.Lock()
@@ -211,7 +204,7 @@ func (t *Time) Observe() <-chan struct{} {
 	return observer
 }
 
-func (t *Time) observe() {
+func (t *Time) observeUnlocked() {
 	for _, observer := range t.observers {
 		close(observer)
 	}

time_test.go

@@ -218,3 +218,30 @@ func TestTime_TickerStop(t *testing.T) {
 		}
 	}
 }
+
+func TestTime_ObserveTick(t *testing.T) {
+	const interval = time.Second
+
+	now := time.Date(2049, 5, 6, 23, 55, 11, 1034, time.UTC)
+	sim := NewTime(now)
+
+	ticker := sim.Ticker(interval)
+	defer ticker.Stop()
+
+	// Check that we do not break existing users of the Time implementation:
+	// the observe channel must be closed on each tick.
+	for range [3]struct{}{} {
+		observe := sim.Observe()
+		sim.Travel(interval)
+		select {
+		case <-ticker.C():
+		default:
+			t.Error("unexpected state")
+		}
+		select {
+		case <-observe:
+		default:
+			t.Error("missing observation on tick")
+		}
+	}
+}

timer.go

@@ -8,14 +8,20 @@ type timer struct {
 	id   int
 }
 
-func (t timer) C() <-chan time.Time {
+func (t *timer) C() <-chan time.Time {
 	return t.ch
 }
 
-func (t timer) Stop() bool {
-	return t.time.stopTimer(t.id)
+func (t *timer) Stop() bool {
+	return t.time.stop(t.id)
 }
 
-func (t timer) Reset(d time.Duration) {
-	t.time.resetTimer(d, t.id, t.ch)
+func (t *timer) Reset(d time.Duration) {
+	t.time.reset(d, t.id, t.do, nil)
+}
+
+// do is the timer’s moment callback. It sends the now time to the underlying
+// channel. Note that do runs under Time’s lock.
+func (t *timer) do(now time.Time) {
+	t.ch <- now
 }