Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

time: Timer.C can still trigger even after Timer.Reset is called #11513

Closed
rogpeppe opened this issue Jul 2, 2015 · 12 comments

Comments

Projects
None yet
8 participants
@rogpeppe
Copy link
Contributor

commented Jul 2, 2015

A common idiom is to keep a single timer and extend its
use by calling Timer.Reset.

From a naive reading of the documentation, these two
lines are equivalent except for saving some garbage:

t.Reset(x)

t := time.NewTimer(x)

Unfortunately t.C is buffered, so if the timer has just expired,
the newly reset timer can actually trigger immediately.

The safe way to do it might be someting like:

t.Stop()
select {
case <-t.C:
default:
}
t.Reset(x)

but this is cumbersome. Perhaps we could change Reset to do this
as a matter of course.

@jbardin

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

If the timer legitimately expired, wouldn't I possibly want that value? Another goroutine could be checking t.C (maybe even counting on the fact that it's buffered), and I'd rather that calling Reset didn't throw that value away.

@enormouspenguin

This comment has been minimized.

Copy link

commented Jul 3, 2015

I don't think that I or many others want that value because firstly, in this case, only a single Timer instance is used in a single goroutine, not being called reset and checked t.C concurrently as your usage intention. Secondly, although it legitimately expired and a time value is already in t.C, but it had been called Reset before any value received from t.C therefore the old time value from t.C is not what user expected. If you keep the old value, you could possibly be interrupted even before you give any chance to allow an operation to proceed.

@enormouspenguin

This comment has been minimized.

Copy link

commented Jul 7, 2015

Even using select statement to mitigate the problem is not enough as the following test (also available on Playground) consistently failed on any machine that has 2 or more real CPU cores:

// testTimer.go
package main

import (
    "runtime"
    "time"
)

func retimer(t *time.Timer, d time.Duration) {
    if !t.Stop() {
        select {
        case <-t.C:
        default:
        }
    }
    t.Reset(d)
}

func runTimer() {
    tmr := time.NewTimer(0)
    retimer(tmr, time.Minute)
    select {
    case <-tmr.C:
        panic("unexpected firing of Timer")
    default:
    }
}

func main() {
    runtime.GOMAXPROCS(2)
    for {
        runTimer()
    }
}
@gopherbot

This comment has been minimized.

Copy link

commented Jul 7, 2015

CL https://golang.org/cl/11960 mentions this issue.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 11, 2015

@cespare

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

@rogpeppe Are you sure your fix is guaranteed to work?

I can reproduce @enormouspenguin's result (no need for the GOMAXPROCS setting in 1.5). You gave this workaround in your original post:

t.Stop()
select {
case <-t.C:
default:
}
t.Reset(x)

However, this is subject to failure because of a race condition; consider the following sequence of events:

  • The timer has just fired, so t.Stop() returns false
  • The value hasn't been delivered to the channel yet, so the default arm of the select is taken

See http://play.golang.org/p/80kzKDerpZ for a demonstration.

Edit: If the timer was previously set but the channel wasn't received from and you want to reliably reset it, do the following:

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

Please ignore the code below.

I think the following is a correct solution:

L:
    for !timer.Stop() {
        select {
        case <-timer.C:
            break L
        default:
        }
        runtime.Gosched() // not needed, but prevents some extra spinning
    }
    timer.Reset(x)
@cespare

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

I feel that the Stop documentation is misleading:

Stop prevents the Timer from firing. It returns true if the call stops the timer, false if the timer has already expired or been stopped. Stop does not close the channel, to prevent a read from the channel succeeding incorrectly.

While technically true, "prevents the Timer from firing" isn't the same as "prevents future channel reads from succeeding", which is what is observable to the user.

It seems to me that the intention of Stop is subtly incompatible with the semantics of buffered channels.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2015

I agree with @jbardin. It's not the library's job to throw away values already sent on the channel. This program changes behavior with the suggested CL:

package main

import "time"

func main() {
    t := time.NewTimer(2 * time.Second)
    time.Sleep(3 * time.Second)
    if t.Reset(2*time.Second) != false {
        panic("expected timer to have fired")
    }
    <-t.C
    <-t.C
}

@rsc rsc closed this Oct 22, 2015

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

This is an unfortunate resolution. A very common use case for Timer.Reset is
to use it as a substitute for time.After in a select loop to avoid creating garbage.
With the current semantics, this is not possible (or at least quite hard) to do reliably.

Consider the following loop:

func run() {
    const timeout = 100 * time.Microsecond
    t := time.NewTimer(timeout)
    for i := 0; i < 10000; i++ {
        select {
        case <-time.After(timeout + time.Duration(rand.Intn(10000) - 5000)):
        case <-t.C:
        }
        t.Reset(timeout)
    }
}

Here the time.After is modelling some external event that fires, and the
timer provides a timeout for that event. As written, the loop does not
work correctly. In a third of cases, the select terminates immediately
because there is a value already in the channel.

The challenge here is to find a way of resetting the timer so that the
select statement is guaranteed not to terminate before 100 microseconds
have passed, and without imposing additional waits in the loop.

None of the solutions presented in this thread (other than changing
Reset itself) are sufficient. The only thing that works is to create a
new timer, which means that Timer.Reset is unfit for the very purpose
it was made for - reducing GC load by reusing the timer.

Here is some code that demonstrates this:
http://play.golang.org/p/u0hkTIE2LN

If we can't fix timer.Reset, perhaps we could add a new method that
provides the semantics that we actually need in this kind of situation.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

I'm sorry, but I didn't really understand the playground code. I think I do understand what you're saying about the initial snippet, though. Here is a variant on it that prints "short sleep" on each iteration that gets a stale value on t.C:

package main

import (
    "fmt"
    "math/rand"
    "time"
)

func main() {
    const timeout = 1 * time.Millisecond
    t := time.NewTimer(timeout)
    for i := 0; i < 100; i++ {
        t2 := timeout + time.Duration(rand.Intn(100)-50)*timeout/100 // t2 in [0.5,1.5)*timeout
        start := time.Now()
        select {
        case <-time.After(t2):
        case <-t.C:
        }
        elapsed := time.Since(start)
        if elapsed < timeout/4 {
            fmt.Println("short sleep:", elapsed)
        }
        t.Reset(timeout)
    }
}

And here is the same program corrected, by using the return value of t.Reset and tracking whether the receive from t.C has happened already:

package main

import (
    "fmt"
    "math/rand"
    "time"
)

func main() {
    const timeout = 1 * time.Millisecond
    t := time.NewTimer(timeout)
    for i := 0; i < 100; i++ {
        t2 := timeout + time.Duration(rand.Intn(100)-50)*timeout/100 // t2 in [0.5,1.5)*timeout
        start := time.Now()
        sawTimeout := false // NEW
        select {
        case <-time.After(t2):
        case <-t.C:
            sawTimeout = true // NEW
        }
        elapsed := time.Since(start)
        if elapsed < timeout/4 {
            fmt.Println("short sleep:", elapsed)
        }
        if !t.Reset(timeout) && !sawTimeout { // NEW
            <-t.C // NEW
        } // NEW
    }
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

@enormouspenguin A closed issue is not the right place to have this kind of conversation. It should happen on the golang-dev mailing list. Thanks.

@enormouspenguin

This comment has been minimized.

Copy link

commented Nov 17, 2015

I deleted my previous inappropriate comment. Thanks for reminding me, @ianlancetaylor.

@RalphCorderoy

This comment has been minimized.

Copy link

commented Nov 29, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.