implement Clock.NewTimer #108
Conversation
rogpeppe
force-pushed
the
014-clock-newtimer
branch
from
fc5ef42
to
d0250be
Compare
rogpeppe
force-pushed
the
014-clock-newtimer
branch
2 times, most recently
from
1b53b06
to
40381bc
Compare
| } | ||
|
|
||
| // AfterFunc is part of the clock.Clock interface. | ||
| func (clock *Clock) AfterFunc(d time.Duration, f func()) clock.Timer { | ||
| return clock.addAlarm(d, nil, func() { | ||
| go f() |
There was a problem hiding this comment.
could/should we use tomb to manage the lifetime of this goroutine? and the stop method should try to cleanup running routines as well.. could we have a test case for this scenario?
There was a problem hiding this comment.
I don't think so, although I'm open to persuasion. Remember that this implementation is solely for testing purposes, and I it's generally better the less that the tests know about the details of the code (whether the code uses AfterFunc vs After), so the test shouldn't need to change if the code uses AfterFunc (we know when it completes) vs After (it triggers something that happens elsewhere).
And we can't make Stop clean up goroutines because that's not something that the stdlib Timer.Stop method provides - it's something that individual clients can do if they need to.
FWIW AfterFunc is very rarely used in the Juju source code - almost always to just update a local variable or field.
clock: add NewTimer This allows mock clocks to use timers in the usual way. This is associated with a companion branch in juju testing (juju/testing#108) which needs this as a dependency update.
Also fix some issues with the current implementation - it's allowable to call Reset after a timer has expired to make the timer fire again, which wouldn't work with the previous implementation.
rogpeppe
force-pushed
the
014-clock-newtimer
branch
from
40381bc
to
b4d2fed
Compare
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-testing |
Also fix some issues with the current implementation - it's allowable
to call Reset after a timer has expired to make the timer fire again,
which wouldn't work with the previous implementation.
Note that this will require a dependency update to use juju/utils#242.
(Review request: http://reviews.vapour.ws/r/5372/)