Reusing time.Timer

[enormouspenguin]

In order to make sure that no leftover/garbage Time value linger inside Timer.C channel after calling Timer.Reset or Timer.Stop, possibly causing unexpected notification, is any of the following snippets the correct way to handle that:

+ Using select statement:

func retimer(t *time.Timer, d time.Duration) {
if t.Reset(d) {

return

}

select {

case <-t.C:

default:

}

}

 + Using len function and chan receive:

func untimer(t *time.Timer) {

if !t.Stop() && len(t.C) != 0 {

<-t.C

}

}

Thanks!

[Matt Harden]

After t.Stop(), the channel will not trigger again. t.C is an unbuffered channel, so len(t.C) is always 0. After t.Reset(d) the timer will not trigger until after duration d. Your two functions above can be replaced with t.Reset(d) and t.Stop() with no change in behavior.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[James Bardin]

On Wednesday, July 1, 2015 at 7:18:25 PM UTC-4, Matt Harden wrote:

After t.Stop(), the channel will not trigger again. t.C is an unbuffered channel, so len(t.C) is always 0. After t.Reset(d) the timer will not trigger until after duration d. Your two functions above can be replaced with t.Reset(d) and t.Stop() with no change in behavior.

No, Timer.C *is* buffered so that it never blocks. http://tip.golang.org/src/time/sleep.go?s=1986:2018#L56

 

On Wed, Jul 1, 2015 at 8:53 AM enormouspenguin <kimkh...@gmail.com> wrote:

In order to make sure that no leftover/garbage Time value linger inside Timer.C channel after calling Timer.Reset or Timer.Stop, possibly causing unexpected notification, is any of the following snippets the correct way to handle that:

+ Using select statement:

func retimer(t *time.Timer, d time.Duration) {
if t.Reset(d) {

return

}

select {

case <-t.C:

default:

}

}

 + Using len function and chan receive:

func untimer(t *time.Timer) {

if !t.Stop() && len(t.C) != 0 {

<-t.C

}

}

If the timer has expired, then there will be (or have been) a Time event in the channel. If for some reason there is a situation where you want to reuse a timer, and it may have expired, and you don't know if the event was consumed, then yes these would work. I wouldn't use len(t.C) though, just use the select with default to avoid the possibility of a race.

[enormouspenguin]

It just occurred to me that I know using len() is racy but might there be a case where the new (just reset) timing duration is short enough that the select statement accidentally consume the latest Time event, not the lingered one we expected to remove? Because when Reset or Stop return false, it's either the last timing event fired or it has been inactive at the time of the call. We could never actually know unless we track it manually.

[Matt Harden]

Yikes! I must have confused it with Ticker. Sorry; ignore everything I said.

I guess it does make sense for C to be buffered on a Timer. One solution is to track whether you have received from C since the last reset. If you haven't, and either Stop() or Reset(d) returns false, then do a receive from C.

An alternative would be to always Stop() before reset, and if Stop returns false, do a non-blocking receive from C before Reset. That way you know Reset will return false and any subsequent receive from C will be after the new delay. You won't have to track whether you had received from C.

[enormouspenguin]

On Thursday, July 2, 2015 at 9:31:38 AM UTC+7, Matt Harden wrote:

An alternative would be to always Stop() before reset, and if Stop returns false, do a non-blocking receive from C before Reset. That way you know Reset will return false and any subsequent receive from C will be after the new delay. You won't have to track whether you had received from C.

Yes, I also have that in mind. Thanks, I think I would go with your suggestion for now.

I still wonder, does the current Timer API broken? Why does Go have to make us jump through hoops and use weird and slowdown tricks just to reuse the Timer. I suspect that much of the performance benefit in reusing Timer is reduced or lost due to having to use workaround like this.

[roger peppe]

I tend to agree that this is a problem.
I've filed an issue: https://github.com/golang/go/issues/11513

[Matt Harden]

Well, I think of Reset as just a way to push out a time out, for instance when you want to timeout on reading from the network, but whenever you get some more data you want to restart the timer. This use case doesn't really need the hoops mentioned. In other cases it's probably cleaner just to create a new timer.

I do think the following could be a much cleaner API:

// AfterChan waits for the duration to elapse and then sends the current time on the channel.
func AfterChan(d Duration, c chan<- Time) {

    // equivalent to the following but implemented more efficiently

    AfterFunc(d, func(){select { case c <- Now(): ; default: }})

}

If you decide to reset you could just call AfterChan again with a new channel and ignore the old channel.

[enormouspenguin]

On Thursday, July 2, 2015 at 11:03:38 PM UTC+7, Matt Harden wrote:

Well, I think of Reset as just a way to push out a time out, for instance when you want to timeout on reading from the network, but whenever you get some more data you want to restart the timer. This use case doesn't really need the hoops mentioned. In other cases it's probably cleaner just to create a new timer.

I agree, but in my case, it's a tight cycle of set, stop sometimes and reset so I think reusing it is more efficient than just creating new Timer or calling time.After() to get a new chan every time.

I do think the following could be a much cleaner API:

// AfterChan waits for the duration to elapse and then sends the current time on the channel.
func AfterChan(d Duration, c chan<- Time) {

    // equivalent to the following but implemented more efficiently

    AfterFunc(d, func(){select { case c <- Now(): ; default: }})

}

If you decide to reset you could just call AfterChan again with a new channel and ignore the old channel.

Cool, but it's still not promote recycle. Also, a simple time.After() would do the trick that AfterChan() tried to accomplish. In either functions, if you reset before expiration and simply just ignore old chan, does the runtime still keep the current timeout schedule running and keep old chan around (not garbage collected it) until it's actually expired? If so, I don't think it's very efficient as we tried to achieve in this discussion.

[Matt Harden]

Yes it would keep the timeout on the schedule and not GC the channel.

--

[enormouspenguin]

I have just found out that select statement is still not enough as my test posted here consistently failed.

[roger peppe]

Interesting. This definitely seems like a more significant bug than we'd
previously thought.

This code (a slight rephrasing of yours) consistently
fails in a few thousand iterations on my machine
under go tip.

http://play.golang.org/p/Ezn4otvG-x

Another odd thing is that calling Reset appears to make a significant
difference to the frequency - if I remove the Reset call, the check
still fails, but only after millions of iterations (66486190 in one run,
146765591 in another). I've reproduced the issue all the way
back to Go 1.0.3.

The cause seems fairly straightforward - the timer function
is not called from runtime.timerproc with the lock held, so there's
a window where the timer can have been removed but the
timer proc is just about to send on the channel. I'm not
entirely sure why the lock is held at that point, because
the function is guaranteed not to block.

Interestingly, I can't reproduce the problem if t.i is set
to -1 when removing the timer in runtime.deltimer,
but I can't see how that could actually be fixing the issue.

I hope we can fix this before Go 1.5.

[roger peppe]

I've filed https://go-review.googlesource.com/#/c/11960
(which might well be bogus - there's quite possibly a really
good reason for dropping the lock while calling the function).

[enormouspenguin]

On Tuesday, July 7, 2015 at 6:47:12 PM UTC+7, rog wrote:

I hope we can fix this before Go 1.5.

I definitely hope so too. But right now, are there any other options to mitigate this problem? 

[Matt Harden]

You could keep track of the last time you reset, and check the time.Time that's returned from the channel; if it's not after the last reset time + delay, throw it away. http://play.golang.org/p/26M3C9S3RA

--

[enormouspenguin]

On Wednesday, July 8, 2015 at 7:59:46 AM UTC+7, Matt Harden wrote:

You could keep track of the last time you reset, and check the time.Time that's returned from the channel; if it's not after the last reset time + delay, throw it away. http://play.golang.org/p/26M3C9S3RA

It sure is ugly hack but it's the only viable solution right now. Guess I should go with it until the real fix comes out. 

[Aliaksandr Valialkin]

I use the following timer pool in gorpc (see https://github.com/valyala/gorpc/blob/master/common.go ) and it seems it works without problems in a production environment generating 40k RPCS per second. I'm wondering will it break in 1.5? :)

var timerPool sync.Pool
 
func acquireTimer(timeout time.Duration) *time.Timer {
    tv := timerPool.Get()
    if tv == nil {
        return time.NewTimer(timeout)
    }
 
    t := tv.(*time.Timer)
    if t.Reset(timeout) {
        panic("BUG: Active timer trapped into acquireTimer()")
    }
    return t
}
 
func releaseTimer(t *time.Timer) {
    if !t.Stop() {
        // Collect possibly added time from the channel
        // if timer has been stopped and nobody collected its' value.
        select {
        case <-t.C:
        default:
        }
    }
 
    timerPool.Put(t)
}

[roger peppe]

This needs to be an unconditional read if it is to work reliably
because the channel send can happen asynchronously after
the Stop returns.

Unfortunately that won't work if you have some other goroutine
reading from the channel when you call Stop.

As pointed out to me earlier today by Dmitry Vyukov
we can't fix the current behaviour because it will break any
programs that are working around the current behaviour
by unconditionally reading from the channel.

This is unfortunate.

[Matt Harden]

How can you work around it by unconditionally reading from the channel? The late write to the channel doesn't always happen, does it?