Timer.Reset

[roger peppe]

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]

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.

[1] https://github.com/golang/go/issues/11513#issuecomment-142410578

--
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]

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]

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]

​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]

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]

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]

10x, got it.

If i understand correctly, select with default case is also racy ?

so i post correct (IMHO) variant:

http://play.golang.org/p/Ys9qqanqmU

 

Djadala

[Russ Cox]

On Mon, Nov 16, 2015 at 7:41 AM roger peppe <rogp...@gmail.com> wrote:

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.

We finally figured this out. Fixed by go.dev/cl/568341, which should ship in Go 1.23.

Best,

Russ 

[roger peppe]

Woo! Thanks for continuing to keep this in mind for all this time.

  cheers,

    rog.

 

Best,

Russ