5
\$\begingroup\$

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.

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?

\$\endgroup\$
3
\$\begingroup\$

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 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.
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Your answer does not appear to be correct (see my own answer). Thanks for the recommendations not directly related to the question though, this kind of help is quite useful at the beginning! \$\endgroup\$
    – marcv81
    Oct 16 '16 at 14:27
  • \$\begingroup\$ @marcv81 - not sure what part of my answer you disagree with. Based on your answer, I think your concern is about draining the Timer's C channel. Note that I don't suggest that you drain it. I only suggest that you Stop the timer before Reset calls on it (which the documentation is very clear about). You are rtight about the documentation being unclear! Also, there's no need to drain the C channel (you're right, it's not initialized, so the drain will "hang"). But, there was no reason to drain the channel ever, so draining it on the Stop would not make sense. \$\endgroup\$
    – rolfl
    Oct 17 '16 at 5:27
  • \$\begingroup\$ You don't directly suggest draining the channel but you quote the doc which says "drain the value from its channel". That's what I disagreed about :) The doc advises to call Stop() before Reset(), but the reasons given appear to relate to draining the channel; I am still not clear whether calling Stop() is required or not. Again, thanks for your participation in this discussion. \$\endgroup\$
    – marcv81
    Oct 17 '16 at 6:54
  • \$\begingroup\$ Actually from the source of Reset() it is clear that calling Stop() beforehand is not required. Reset() calls Stop() in any case, but does not perform the channel drain. Hence calling Stop() before Reset() is only required if the channel requires draining, which is not the case for Timers created with AfterFunc(). \$\endgroup\$
    – marcv81
    Oct 17 '16 at 7:01
1
\$\begingroup\$

I found the answer to my own question thanks to the code for NewTimer() and AfterFunc() being available and well documented.

Timer contains a channel and a runtimeTimer. runtimeTimer appears to be implemented in C or C++ (the comments mention runtime.h), but I could not find the source. runtimeTimer handles most of the Timer logic. Among other fields, runtimeTimer contains a function to call when the duration elapses.

  • NewTimer() creates a Timer an initializes its channel. The underlying runtimeTimer function is set to sendTime(). When the duration elapses, sendTime() sends the current time through the channel.

  • AfterTime() creates a Timer but does not initialize its channel. The underlying runtimeTimer function is directly set to the user-defined function (I'm lying slightly, please see goFunc() if interested). The channel is not used at any point.

TL;DR: When using AfterFunc() to create a Timer one must not drain the channel. The doc could be clearer.

Source code: https://golang.org/src/time/sleep.go

EDIT

The original answer did not address whether calling Stop() before Reset() on the Timer is required or not. We can see in the source that Reset() does call Stop() in any case. The documentation and code comments recommend to call Stop() before Reset(), but this is only required so that the channel can be drained. When creating a Timer with AfterFunc() we must not attempt to drain the channel, hence calling Stop() before Reset() is not required.

\$\endgroup\$
2
  • \$\begingroup\$ While your investigation of the source code is neat, calling Stop before Resetis still "safe" to do, and there may be other reasons to follow the documentation for the call for example there may be plans for future changes that require that call (even if the current implementation allows otherwise). Note that the Go specification is described in the documentation, not the implementation. \$\endgroup\$
    – rolfl
    Oct 17 '16 at 11:17
  • \$\begingroup\$ Very fair point about specification vs. implementation. \$\endgroup\$
    – marcv81
    Oct 17 '16 at 14:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.