Timer.Reset

2197 views
Skip to first unread message

roger peppe

unread,
Nov 16, 2015, 7:41:06 AM11/16/15
to golang-dev
I just replied to this issue #11513 which
was closed recently. Am I wrong to expect
that we might have a primitive that
can let timers be reused in this way?

I'm mentioning this here because I suspect many people
are unaware of the problem and perhaps there
is actually a decent way to work around the issue
that I haven't figured out.

https://github.com/golang/go/issues/11513

Peter Waller

unread,
Nov 16, 2015, 10:22:10 AM11/16/15
to roger peppe, golang-dev, Caleb Spare
Your email made me aware of the problem and helped me discover a race in some code I recently wrote that uses Timer in exactly this manner. This hazard is not clear from the documentation. I had figured out that I needed to make non-blocking read from timer.C after calling Stop(), but not that delivery might come an arbitrarily long time after Stop().

I'm also confused by Caleb's solution [1]:
L:
    for !timer.Stop() {
        select {
        case <-timer.C:
            break L
        default:
        }
        runtime.Gosched() // not needed, but prevents some extra spinning
    }
    timer.Reset(x)
If timer.Stop() returns false, why will it ever return true again (causing a the loop to halt via the loop condition), unless timer.C is eventually readable? If timer.C is eventually readable, why not just do a blocking <-timer.C? In that case, I guess you race against whoever else might be reading timer.C. It seems that the loop L might block in that case.




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

Russ Cox

unread,
Nov 16, 2015, 12:32:56 PM11/16/15
to Peter Waller, roger peppe, golang-dev, Caleb Spare
On Mon, Nov 16, 2015 at 10:21 AM, Peter Waller <pe...@scraperwiki.com> wrote:
Your email made me aware of the problem and helped me discover a race in some code I recently wrote that uses Timer in exactly this manner. This hazard is not clear from the documentation. I had figured out that I needed to make non-blocking read from timer.C after calling Stop(), but not that delivery might come an arbitrarily long time after Stop().

It's not arbitrarily long. If Stop returns false and you haven't received from timer.C yet, then a plain "<-timer.C" will run quickly, subject only to scheduling delays.
 
I'm also confused by Caleb's solution [1]:
L:
    for !timer.Stop() {
        select {
        case <-timer.C:
            break L
        default:
        }
        runtime.Gosched() // not needed, but prevents some extra spinning
    }
    timer.Reset(x)
If timer.Stop() returns false, why will it ever return true again (causing a the loop to halt via the loop condition), unless timer.C is eventually readable? If timer.C is eventually readable, why not just do a blocking <-timer.C? In that case, I guess you race against whoever else might be reading timer.C. It seems that the loop L might block in that case.

Yes, that's confusing. I think the code was just trying to be clever or short or both. At most only the very first call to timer.Stop can return true. All subsequent calls are guaranteed to be no-ops and must return false. The loop is equivalent to saying if !timer.Stop() { L: for { ... } }.

Stepping back, the loop is also unnecessary, unless you are racing with another goroutine receiving from timer.C. In that case, such a loop is necessary if you want to make sure no values are in flight, but I am not sure there are many good reasons to be doing <-timer.C in one goroutine and call timer.Stop/timer.Reset in a different one. That's going to be hard to reason about no matter what.

If you're not racing with a pending receive on timer.C, then you can use the code I just posted in response to Roger, namely:

    if !timer.Stop() && !alreadyReadFromTimerC {
        <-timer.C
    }

The body of the if statement will not block for longer than scheduling delays.

Russ

roger peppe

unread,
Nov 16, 2015, 1:41:24 PM11/16/15
to Russ Cox, Peter Waller, golang-dev, Caleb Spare
On 16 November 2015 at 17:32, Russ Cox <r...@golang.org> wrote:
> On Mon, Nov 16, 2015 at 10:21 AM, Peter Waller <pe...@scraperwiki.com>
> wrote:
>>
>> Your email made me aware of the problem and helped me discover a race in
>> some code I recently wrote that uses Timer in exactly this manner. This
>> hazard is not clear from the documentation. I had figured out that I needed
>> to make non-blocking read from timer.C after calling Stop(), but not that
>> delivery might come an arbitrarily long time after Stop().
>
>
> It's not arbitrarily long. If Stop returns false and you haven't received
> from timer.C yet, then a plain "<-timer.C" will run quickly, subject only to
> scheduling delays.

Ok, of course, it's clear in retrospect: it's not possible to make a
Timer.Reset substitute that works as I'd like (convenient for fixing
bugs in existing code), but at least it's possible
to work around the situation by stopping the timer when it's known that
the timer has expired.

I still think the API is harder to use for this common case than it should
be. Perhaps we can mitigate the situation by documenting this
pattern well.

Caleb Spare

unread,
Nov 16, 2015, 9:05:02 PM11/16/15
to Russ Cox, Peter Waller, roger peppe, golang-dev
​Thanks Peter and Russ. I'm not sure what I was thinking when I wrote that, looking at it now.

I have updated that comment to recommend doing simply

if !timer.Stop() {
  <-timer.C
}
timer.Reset(x)

to avoid confusing people in the future.

-Caleb​
 

dja...@gmail.com

unread,
Nov 17, 2015, 5:18:03 AM11/17/15
to golang-dev, r...@golang.org, pe...@scraperwiki.com, rogp...@gmail.com
did you try  http://play.golang.org/p/cGnlO9vy5f ?
result is:
fatal error: all goroutines are asleep - deadlock!
i think it should work,  may be there is race condition in timer.Stop ?

if i add select with default clause, it works & dont print "short sleep"
http://play.golang.org/p/sO55XcYYRZ

i dont understand how read from timer channel can block after stop return false ? 

Djadala

Caleb Spare

unread,
Nov 17, 2015, 5:24:48 AM11/17/15
to dja...@gmail.com, golang-dev, Russ Cox, Peter Waller, roger peppe
On Tue, Nov 17, 2015 at 2:17 AM, <dja...@gmail.com> wrote:
did you try  http://play.golang.org/p/cGnlO9vy5f ?
result is:
fatal error: all goroutines are asleep - deadlock!
i think it should work,  may be there is race condition in timer.Stop ?

​If you've already pulled the value off timer.C, then

if !timer.Stop() {
  <-timer.C
}

will deadlock. If you need it, track whether you did as in Russ's alreadyReadFromTimerC variable.
 

if i add select with default clause, it works & dont print "short sleep"
http://play.golang.org/p/sO55XcYYRZ

i dont understand how read from timer channel can block after stop return false ?

​Stop doesn't close the channel, so receiving from timer.C twice without Resetting will block forever.
 
 
--

dja...@gmail.com

unread,
Nov 17, 2015, 6:11:14 AM11/17/15
to golang-dev, dja...@gmail.com, r...@golang.org, pe...@scraperwiki.com, rogp...@gmail.com
10x, got it.
If i understand correctly, select with default case is also racy ?

so i post correct (IMHO) variant:
 
Djadala
Reply all
Reply to author
Forward
0 new messages