patterngoMinor
Watchdog in Golang
Viewed 0 times
golangwatchdogstackoverflow
Problem
I am new to Go. I am trying to implement a simple watchdog. It calls a function once after a certain delay has elapsed; kicking it resets the delay. Kicking it after the function has already been called restarts it.
I read https://golang.org/pkg/time/ and understand there might be some caveats around
Could anyone please let me know if anything is wrong with the code? Please provide test cases (plain English is okay) to crash it or get an unexpected behaviour. Can you spot any inefficiencies?
type watchdog struct {
interval time.Duration
timer *time.Timer
}
func NewWatchdog(interval time.Duration, callback func()) *watchdog {
w := watchdog{
interval: interval,
timer: time.AfterFunc(interval, callback),
}
return &w
}
func (w *watchdog) Stop() {
w.timer.Stop()
}
func (w *watchdog) Kick() {
w.timer.Reset(w.interval)
}I read https://golang.org/pkg/time/ and understand there might be some caveats around
func (*Timer) Reset. My code is totally oblivious of this fact and seems to work nonetheless. I tested a few corner cases: calling Stop() twice in a row, calling Kick() after Stop() or after the function has been called.Could anyone please let me know if anything is wrong with the code? Please provide test cases (plain English is okay) to crash it or get an unexpected behaviour. Can you spot any inefficiencies?
Solution
Go has a concurrency model that's different to most other languages. You'll get used to it, but the instructions for the usage of the
You should follow those instructions.
To reuse an active timer, always call its Stop method first and—if it had expired—drain the value from its channel.
As you have discovered, in most cases, the consequence of not following the instructions are not significant. Where it will become significant is when the timer event happens at (about) the same time as the reset call. You can't create a test case that forces that timing, but at some point, in the future, the timing will happen to coincide, and your program's behaviour will be odd (the timer will tick even though you're calling the reset event, for example).
In your use-case, this concurrency problem may still not be significant. It will create "false positives" - watchdog timeouts for apparently reset watchdogs - but the reset was so close to the watchdog timeout that the watchdog cycle was probably worth triggering anyway.
As for your actual code, it's pretty good. There are some small things to consider.
Firstly, why don't you make it a separate package? At the moment, your
Note that the creation method is renamed to just
Finally, I understand why you have the callback function argument/mechanism for your watchdog. There's nothing wrong with that, but there will/may be times when a user of the watchdog would prefer a channel output instead of a callback. Having a channel allows people to merge the watchdog timeout logic with other active logic. For example, a loop like:
timer.Reset() method are very specific, and are a consequence of Go's concurrency model.You should follow those instructions.
To reuse an active timer, always call its Stop method first and—if it had expired—drain the value from its channel.
As you have discovered, in most cases, the consequence of not following the instructions are not significant. Where it will become significant is when the timer event happens at (about) the same time as the reset call. You can't create a test case that forces that timing, but at some point, in the future, the timing will happen to coincide, and your program's behaviour will be odd (the timer will tick even though you're calling the reset event, for example).
In your use-case, this concurrency problem may still not be significant. It will create "false positives" - watchdog timeouts for apparently reset watchdogs - but the reset was so close to the watchdog timeout that the watchdog cycle was probably worth triggering anyway.
As for your actual code, it's pretty good. There are some small things to consider.
Firstly, why don't you make it a separate package? At the moment, your
watchdog is not exported, and can only be run by things in your package. You export the Stop and Kick methods, but not the actual struct. Create a watchdog folder, and then make a standalone package:package watchdog;
import (
"time"
)
type Watchdog struct {
interval time.Duration
timer *time.Timer
}
func New(interval time.Duration, callback func()) *Watchdog {
w := Watchdog{
interval: interval,
timer: time.AfterFunc(interval, callback),
}
return &w
}
func (w *watchdog) Stop() {
w.timer.Stop()
}
func (w *watchdog) Kick() {
w.timer.Stop()
w.timer.Reset(w.interval)
}Note that the creation method is renamed to just
New. Thus calling code can just:import "watchdog"
wdog := watchdog.New(time.Second, func() {....})
....
wdog.Kick()Finally, I understand why you have the callback function argument/mechanism for your watchdog. There's nothing wrong with that, but there will/may be times when a user of the watchdog would prefer a channel output instead of a callback. Having a channel allows people to merge the watchdog timeout logic with other active logic. For example, a loop like:
wdog := watchdog.New(time.Second)
for {
wdog.Kick()
select {
case <- wdog:
// watchdog expired. Reset entire system
case <-input:
// got data so process it.
}
}Code Snippets
package watchdog;
import (
"time"
)
type Watchdog struct {
interval time.Duration
timer *time.Timer
}
func New(interval time.Duration, callback func()) *Watchdog {
w := Watchdog{
interval: interval,
timer: time.AfterFunc(interval, callback),
}
return &w
}
func (w *watchdog) Stop() {
w.timer.Stop()
}
func (w *watchdog) Kick() {
w.timer.Stop()
w.timer.Reset(w.interval)
}import "watchdog"
wdog := watchdog.New(time.Second, func() {....})
....
wdog.Kick()wdog := watchdog.New(time.Second)
for {
wdog.Kick()
select {
case <- wdog:
// watchdog expired. Reset entire system
case <-input:
// got data so process it.
}
}Context
StackExchange Code Review Q#144273, answer score: 3
Revisions (0)
No revisions yet.