-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Closed
Labels
FrozenDueToAgeNeedsDecisionFeedback is required from experts, contributors, and/or the community before a change can be made.Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Description
I saw two common usage patterns of Timer.Reset:
First is ensuring that a sequence of events happens in a timely fashion:
timeout := time.NewTimer(T)
for {
if !timeout.Reset(T) {
<-timeout.C
}
select {
case <- ...: ...
case <-timeout.C: ...
}
}Second is connection timeout with an ability to change the timeout:
timeout := time.NewTimer(T)
// goroutine 1
for {
select {
case <- ...: ...
case <-timeout.C: ...
}
}
// goroutine 2
if !timeout.Reset(T2) {
<-timeout.C
}There are several things that can go wrong here depending on timings:
- Reset returns false; one element is already in the channel (the old one); timer goroutine sends another (corresponding to the new timeout), it is discarded since the channel is full; now we drain the channel to read out the old value; as the result timeout will never happen as the new value was discarded.
- Reset returns false; one element is already in the channel (the old one); timer goroutine sends another (corresponding to the new timeout), it is discarded since the channel is full; goroutine 1 reads out from the channel; goroutine 2 hangs forever trying to drain the channel.
I've found several cases of this bug in our internal codebase, one even with a TODO by @bradfitz describing this race.
Timer.Reset does the following:
func (t *Timer) Reset(d Duration) bool {
if t.r.f == nil {
panic("time: Reset called on uninitialized Timer")
}
w := when(d)
active := stopTimer(&t.r)
t.r.when = w
startTimer(&t.r)
return active
}What would be possible to use correctly is:
func (t *Timer) Reset2(d Duration) bool {
if t.r.f == nil {
panic("time: Reset called on uninitialized Timer")
}
w := when(d)
active := stopTimer(&t.r)
if !active {
<-t.C
}
t.r.when = w
startTimer(&t.r)
return active
}Metadata
Metadata
Assignees
Labels
FrozenDueToAgeNeedsDecisionFeedback is required from experts, contributors, and/or the community before a change can be made.Feedback is required from experts, contributors, and/or the community before a change can be made.