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 · 13 comments
Closed

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

rogpeppe opened this issue Jul 2, 2015 · 13 comments

Comments

@rogpeppe
Copy link
Contributor

rogpeppe 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
Copy link
Contributor

jbardin 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
Copy link

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
Copy link

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
Copy link

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

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 11, 2015
@cespare
Copy link
Contributor

cespare 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
Copy link
Contributor

cespare 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
Copy link
Contributor

rsc 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 as completed Oct 22, 2015
@rogpeppe
Copy link
Contributor Author

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
Copy link
Contributor

rsc 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
Copy link
Contributor

@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
Copy link

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

@RalphCorderoy
Copy link

@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

Yesterday I landed go.dev/cl/568341, which fixes this bug.

@rsc rsc reopened this Mar 15, 2024
@rsc rsc closed this as completed Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants