Go reset a timer.NewTimer within select loop

Question

I have a scenario in which I'm processing events on a channel, and one of those events is a heartbeat which needs to occur within a certain timeframe. Events which are not heartbeats will continue consuming the timer, however whenever the heartbeat is received I want to reset the timer. The obvious way to do this would be by using a time.NewTimer.

For example:

func main() {
    to := time.NewTimer(3200 * time.Millisecond)
    for {
        select {
        case event, ok := <-c:
            if !ok {
                return
            } else if event.Msg == "heartbeat" {
                to.Reset(3200 * time.Millisecond)               
            }
        case remediate := <-to.C:
            fmt.Println("do some stuff ...")
            return
        }
    }
}

Note that a time.Ticker won't work here as the remediation should only be triggered if the heartbeat hasn't been received, not every time.

The above solution works in the handful of low volume tests I've tried it on, however I came across a Github issue indicating that resetting a Timer which has not fired is a no-no. Additionally the documentation states:

Reset should be invoked only on stopped or expired timers with drained channels. If a program has already received a value from t.C, the timer is known to have expired and the channel drained, so t.Reset can be used directly. If a program has not yet received a value from t.C, however, the timer must be stopped and—if Stop reports that the timer expired before being stopped—the channel explicitly drained:

if !t.Stop() {
    <-t.C
}
t.Reset(d)

This gives me pause, as it seems to describe exactly what I'm attempting to do. I'm resetting the Timer whenever the heartbeat is received, prior to it having fired. I'm not experienced enough with Go yet to digest the whole post, but it certainly seems like I may be headed down a dangerous path.

One other solution I thought of is to simply replace the Timer with a new one whenever the heartbeat occurs, e.g:

else if event.Msg == "heartbeat" {
                to = time.NewTimer(3200 * time.Millisecond)               
            }

At first I was worried that the rebinding to = time.NewTimer(3200 * time.Millisecond) wouldn't be visible within the select:

For all the cases in the statement, the channel operands of receive operations and the channel and right-hand-side expressions of send statements are evaluated exactly once, in source order, upon entering the "select" statement. The result is a set of channels to receive from or send to, and the corresponding values to send.

But in this particular case since we are inside a loop, I would expect that upon each iteration we re-enter select and therefore the new binding should be visible. Is that a fair assumption?

I realize there are similar questions out there, and I've tried to read the relevant posts/documentation, but I am new to Go just want to be sure I'm understanding things correctly here.

So my questions are:

• Is my use of timer.Reset() unsafe, or are the cases mentioned in the Github issue highlighting other problems which are not applicable here? Is the explanation in the docs cryptic or do I just need more experience with Go?

• If it is unsafe, is my second proposed solution acceptable (rebinding the timer on each iteration).

ADDENDUM

---

Upon further reading, most of the pitfalls outlined in the issues are describing scenarios in which the timer has already fired (placing a result on the channel), and subsequent to that firing some other process attempts to Reset it. For this narrow case, I understand the need to test with !t.Stop() since a false return of Stop would indicate the timer has already fired, and as such must be drained prior to calling Reset.

What I still do not understand, is why it is necessary to call t.Stop() prior to t.Reset(), when the Timer has yet to fire. None of the examples go into that as far as I can tell.

Answer 1

I've faced a similar issue. After reading a lot of information, I came up with a solution that goes along these lines:

package main

import (
    "fmt"
    "time"
)

func main() {
    const timeout = 2 * time.Second

    // Prepare a timer that is stopped and ready to be reset.
    // Stop will never return false, because an hour is too long
    // for timer to fire. Thus there's no need to drain timer.C.
    timer := time.NewTimer(timeout)
    timer.Stop()

    // Make sure to stop the timer when we return.
    defer timer.Stop()

    // This variable is needed because we need to track if we can safely reset the timer
    // in a loop. Calling timer.Stop() will return false on every iteration, but we can only
    // drain the timer.C once, otherwise it will deadlock.
    var timerSet bool

    c := make(chan time.Time)

    // Simulate events that come in every second
    // and every 5th event delays so that timer can fire.
    go func() {
        var i int
        ticker := time.NewTicker(1 * time.Second)
        defer ticker.Stop()

        for t := range ticker.C {
            i++
            if i%5 == 0 {
                fmt.Println("Sleeping")
                time.Sleep(3 * time.Second)
            }

            c <- t

            if i == 20 {
                break
            }
        }
        close(c)
    }()

    for {
        select {
        case t, ok := <-c:
            if !ok {
                fmt.Println("Closed channel")
                return
            }
            fmt.Println("Got event", t, timerSet)

            // We got an event, and timer was already set.
            // We need to stop the timer and drain the channel if needed,
            // so that we can safely reset it later.
            if timerSet {
                if !timer.Stop() {
                    <-timer.C
                }
                timerSet = false
            }

            // If timer was not set, or it was stopped before, it's safe to reset it.
            if !timerSet {
                timerSet = true
                timer.Reset(timeout)
            }
        case remediate := <-timer.C:
            fmt.Println("Timeout", remediate)
            // It's important to store that timer is not set anymore.
            timerSet = false
        }
    }
}

Link to playground: https://play.golang.org/p/0QlujZngEGg

Answer 2

What I still do not understand, is why it is necessary to call t.Stop() prior to t.Reset(), when the Timer has yet to fire.

The "when the Timer has yet to fire" bit is critical here. The timer fires within a separate go routine (part of the runtime) and this can happen at any time. You have no way of knowing whether the timer has fired at the time you call to.Reset(3200 * time.Millisecond) (it may even fire while that function is running!).

Here is an example that demonstrates this and is somewhat similar to what you are attempting (based on this):

func main() {
    eventC := make(chan struct{}, 1)
    go keepaliveLoop(eventC )

    // Reset the timer 1000 (approx) times; once every millisecond (approx)
    // This should prevent the timer from firing (because that only happens after 2 ms)
    for i := 0; i < 1000; i++ {
        time.Sleep(time.Millisecond)
        // Don't block if there is already a reset request
        select {
        case eventC <- struct{}{}:
        default:
        }
    }
}

func keepaliveLoop(eventC chan struct{}) {
    to := time.NewTimer(2 * time.Millisecond)

    for {
        select {
        case <-eventC: 
            //if event.Msg == "heartbeat"...
            time.Sleep(3 * time.Millisecond) // Simulate reset work (delay could be partly dur to whatever is triggering the
            to.Reset(2 * time.Millisecond)
        case <-to.C:
            panic("this should never happen")
        }
    }
}

Try it in the playground.

This may appear contrived due to the time.Sleep(3 * time.Millisecond) but that is just included to consistently demonstrate the issue. Your code may work 99.9% of the time but there is always the possibility that both the event and timer channels will fire before the select is run (in which a random case will run) or while the code in the case event, ok := <-c: block is running (including while Reset() is in progress). The result of this happening would be unexpected calls of the remediate code (which may not be a big issue).

Fortunately solving the issue is relatively easy (following the advice in the documentation):

time.Sleep(3 * time.Millisecond) // Simulate reset work (delay could be partly dur to whatever is triggering the
if !to.Stop() {
    <-to.C
}
to.Reset(2 * time.Millisecond)

Try this in the playground.

This works because to.Stop returns "true if the call stops the timer, false if the timer has already expired or been stopped". Note that things get a more complicated if the timer is used in multiple go-routines "This cannot be done concurrent to other receives from the Timer's channel or other calls to the Timer's Stop method" but this is not the case in your use-case.

Is my use of timer.Reset() unsafe, or are the cases mentioned in the Github issue highlighting other problems which are not applicable here?

Yes - it is unsafe. However the impact is fairly low. The event arriving and timer triggering would need to happen almost concurrently and, in that case, running the remediate code might not be a big issue. Note that the fix is fairly simple (as per the docs)

If it is unsafe, is my second proposed solution acceptable (rebinding the timer on each iteration).

Your second proposed solution also works (but note that the garbage collector cannot free the timer until after it has fired, or been stopped, which may cause issues if you are creating timers rapidly).

Note: Re the suggestion from @JotaSantos

Another thing that could be done is to add a select when draining <-to.C (on the Stop "if") with a default clause. That would prevent the pause.

See this comment for details of why this may not be a good approach (it's also unnecessary in your situation).