Skip to content

Commit

Permalink
Fix incorrect time sent on test Timer channel (temporalio#6380)
Browse files Browse the repository at this point in the history
## What changed?
<!-- Describe what has changed in this PR -->
Updates the `NewTimer` logic in our clock testing package to ensure that
the correct time is sent on a `Timer`'s channel, even after `Reset`

## Why?
<!-- Tell your future self why have you made these changes -->
This is the behavior of `Timer` in the stdlib, so it matches
expectations of users

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Updated unit tests

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
None, this only affects tests

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
  • Loading branch information
jprieto-temporal authored Aug 8, 2024
1 parent 89530ce commit 6b4dbde
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 13 deletions.
35 changes: 25 additions & 10 deletions common/clock/event_time_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type (
done bool
// index of the timer in the parent timeSource
index int
// channel on which the current time is sent when a timer fires
c chan time.Time
}
)

Expand Down Expand Up @@ -81,18 +83,12 @@ func (ts *EventTimeSource) Since(t time.Time) time.Duration {
// wrap all such calls in a goroutine. If the duration is non-positive, the callback will fire immediately before
// AfterFunc returns.
func (ts *EventTimeSource) AfterFunc(d time.Duration, f func()) Timer {
ts.mu.Lock()
defer ts.mu.Unlock()

if d < 0 {
d = 0
}
t := &fakeTimer{timeSource: ts, deadline: ts.now.Add(d), callback: f}
t.index = len(ts.timers)
ts.timers = append(ts.timers, t)
ts.fireTimers()

return t
timer := &fakeTimer{timeSource: ts, deadline: ts.Now().Add(d), callback: f}
ts.addTimer(timer)
return timer
}

// NewTimer creates a Timer that will send the current time on a channel after at least
Expand All @@ -101,7 +97,22 @@ func (ts *EventTimeSource) NewTimer(d time.Duration) (<-chan time.Time, Timer) {
c := make(chan time.Time, 1)
// we can't call ts.Now() from the callback so just calculate what it should be
target := ts.Now().Add(d)
return c, ts.AfterFunc(d, func() { c <- target })
timer := &fakeTimer{
timeSource: ts,
deadline: target,
callback: func() { c <- target },
c: c,
}
ts.addTimer(timer)
return c, timer
}

func (ts *EventTimeSource) addTimer(t *fakeTimer) {
ts.mu.Lock()
defer ts.mu.Unlock()
t.index = len(ts.timers)
ts.timers = append(ts.timers, t)
ts.fireTimers()
}

// Update the fake current time. It returns the timeSource so that you can chain calls like this:
Expand Down Expand Up @@ -163,6 +174,10 @@ func (t *fakeTimer) Reset(d time.Duration) bool {
t.done = false
t.index = len(t.timeSource.timers)
t.timeSource.timers = append(t.timeSource.timers, t)
// Only reset the callback if this timer was created via NewTimer
if t.c != nil {
t.callback = func() { t.c <- t.deadline }
}
}
t.timeSource.fireTimers()
return wasActive
Expand Down
23 changes: 20 additions & 3 deletions common/clock/event_time_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,13 @@ func TestEventTimeSource_Update(t *testing.T) {
ev2.AssertFiredOnce("Timer should fire after deadline")
}

func TestEventTimeSource_NewTimer(t *testing.T) {
func TestEventTimeSource_NewTimerWithChannelAndReset(t *testing.T) {
t.Parallel()

source := clock.NewEventTimeSource()

ch, _ := source.NewTimer(time.Second)
ch, timer := source.NewTimer(time.Second)
expectedFireTime := source.Now().Add(time.Second)

select {
case <-ch:
Expand All @@ -239,8 +240,24 @@ func TestEventTimeSource_NewTimer(t *testing.T) {

source.Advance(2 * time.Second)

// Since the timer duration was 1s, it should send the time at which the timer fired (which was 1s ago) on the channel
select {
case <-ch:
case result := <-ch:
assert.Equal(t, expectedFireTime, result)
default:
t.Error("should have fired")
}

// Reset the timer so that it fires in 1 second
timer.Reset(time.Second)
expectedFireTime = source.Now().Add(time.Second)

source.Advance(2 * time.Second)

// Check that the timer sends the time at which it fired on the channel
select {
case result := <-ch:
assert.Equal(t, expectedFireTime, result)
default:
t.Error("should have fired")
}
Expand Down

0 comments on commit 6b4dbde

Please sign in to comment.