8000 runtime: if stop/reset races with running timer, return correct result · golang/go@2ebaff4 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2ebaff4

Browse files
ianlancetaylorgopherbot
ianlancetaylor
authored and
gopherbot
committed
runtime: if stop/reset races with running timer, return correct result
The timer code is careful to ensure that if stop/reset is called while a timer is being run, we cancel the run. However, the code failed to ensure that in that case stop/reset returned true, meaning that the timer had been stopped. In the racing case stop/reset could see that t.when had been set to zero, and return false, even though the timer had not and never would fire. Fix this by tracking whether a timer run is in progress, and using that to reliably detect that the run was cancelled, meaning that stop/reset should return true. Fixes #69312 Change-Id: I78e870063eb96650638f12c056e32c931417c84a Reviewed-on: https://go-review.googlesource.com/c/go/+/611496 Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Ian Lance Taylor <iant@golang.org>
1 parent 3587430 commit 2ebaff4

File tree

2 files changed

+139
-5
lines changed

2 files changed

+139
-5
lines changed

‎src/runtime/time.go‎

Lines changed: 77 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,40 @@ type timer struct {
2626
// mu protects reads and writes to all fields, with exceptions noted below.
2727
mu mutex
2828

29-
astate atomic.Uint8 // atomic copy of state bits at last unlock
30-
state uint8 // state bits
31-
isChan bool // timer has a channel; immutable; can be read without lock
32-
blocked uint32 // number of goroutines blocked on timer's channel
29+
astate atomic.Uint8 // atomic copy of state bits at last unlock
30+
state uint8 // state bits
31+
isChan bool // timer has a channel; immutable; can be read without lock
32+
33+
// isSending is used to handle races between running a
34+
// channel timer and stopping or resetting the timer.
35+
// It is used only for channel timers (t.isChan == true).
36+
// The lowest zero bit is set when about to send a value on the channel,
37+
// and cleared after sending the value.
38+
// The stop/reset code uses this to detect whether it
39+
// stopped the channel send.
40+
//
41+
// An isSending bit is set only when t.mu is held.
42+
// An isSending bit is cleared only when t.sendLock is held.
43+
// isSending is read only when both t.mu and t.sendLock are held.
44+
//
45+
// Setting and clearing Uint8 bits handles the case of
46+
// a timer that is reset concurrently with unlockAndRun.
47+
// If the reset timer runs immediately, we can wind up with
48+
// concurrent calls to unlockAndRun for the same timer.
49+
// Using matched bit set and clear in unlockAndRun
50+
// ensures that the value doesn't get temporarily out of sync.
51+
//
52+
// We use a uint8 to keep the timer struct small.
53+
// This means that we can only support up to 8 concurrent
54+
// runs of a timer, where a concurrent run can only occur if
55+
// we start a run, unlock the timer, the timer is reset to a new
56+
// value (or the ticker fires again), it is ready to run,
57+
// and it is actually run, all before the first run completes.
58+
// Since completing a run is fast, even 2 concurrent timer runs are
59+
// nearly impossible, so this should be safe in practice.
60+
isSending atomic.Uint8
61+
62+
blocked uint32 // number of goroutines blocked on timer's channel
3363

3464
// Timer wakes up at when, and then at when+period, ... (period > 0 only)
3565
// each time calling f(arg, seq, delay) in the timer goroutine, so f must be
@@ -431,6 +461,15 @@ func (t *timer) stop() bool {
431461
// Stop any future sends with stale values.
432462
// See timer.unlockAndRun.
433463
t.seq++
464+
465+
// If there is currently a send in progress,
466+
// incrementing seq is going to prevent that
467+
// send from actually happening. That means
468+
// that we should return true: the timer was
469+
// stopped, even though t.when may be zero.
470+
if t.isSending.Load() > 0 {
471+
pending = true
472+
}
434473
}
435474
t.unlock()
436475
if !async && t.isChan {
@@ -525,6 +564,15 @@ func (t *timer) modify(when, period int64, f func(arg any, seq uintptr, delay in
525564
// Stop any future sends with stale values.
526565
// See timer.unlockAndRun.
527566
t.seq++
567+
568+
// If there is currently a send in progress,
569+
// incrementing seq is going to prevent that
570+
// send from actually happening. That means
571+
// that we should return true: the timer was
572+
// stopped, even though t.when may be zero.
573+
if t.isSending.Load() > 0 {
574+
pending = true
575+
}
528576
}
529577
t.unlock()
530578
if !async && t.isChan {
@@ -1013,6 +1061,24 @@ func (t *timer) unlockAndRun(now int64) {
10131061
}
10141062
t.updateHeap()
10151063
}
1064+
1065+
async := debug.asynctimerchan.Load() != 0
1066+
var isSendingClear uint8
1067+
if !async && t.isChan {
1068+
// Tell Stop/Reset that we are sending a value.
1069+
// Set the lowest zero bit.
1070+
// We do this awkward step because atomic.Uint8
1071+
// doesn't support Add or CompareAndSwap.
1072+
// We only set bits with t locked.
1073+
v := t.isSending.Load()
1074+
i := sys.TrailingZeros8(^v)
1075+
if i == 8 {
1076+
throw("too many concurrent timer firings")
1077+
}
1078+
isSendingClear = 1 << i
1079+
t.isSending.Or(isSendingClear)
1080+
}
1081+
10161082
t.unlock()
10171083

10181084
if raceenabled {
@@ -1028,7 +1094,6 @@ func (t *timer) unlockAndRun(now int64) {
10281094
ts.unlock()
10291095
}
10301096

1031-
async := debug.asynctimerchan.Load() != 0
10321097
if !async && t.isChan {
10331098
// For a timer channel, we want to make sure that no stale sends
10341099
// happen after a t.stop or t.modify, but we cannot hold t.mu
@@ -1044,6 +1109,10 @@ func (t *timer) unlockAndRun(now int64) {
10441109
// and double-check that t.seq is still the seq value we saw above.
10451110
// If not, the timer has been updated and we should skip the send.
10461111
// We skip the send by reassigning f to a no-op function.
1112+
//
1113+
// The isSending field tells t.stop or t.modify that we have
1114+
// started to send the value. That lets them correctly return
1115+
// true meaning that no value was sent.
10471116
lock(&t.sendLock)
10481117
if t.seq != seq {
10491118
f = func(any, uintptr, int64) {}
@@ -1053,6 +1122,9 @@ func (t *timer) unlockAndRun(now int64) {
10531122
f(arg, seq, delay)
10541123

10551124
if !async && t.isChan {
1125+
// We are no longer sending a value.
1126+
t.isSending.And(^isSendingClear)
1127+
10561128
unlock(&t.sendLock)
10571129
}
10581130

‎src/time/sleep_test.go‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,68 @@ func TestAdjustTimers(t *testing.T) {
785785
}
786786
}
787787

788+
func TestStopResult(t *testing.T) {
789+
testStopResetResult(t, true)
790+
}
791+
792+
func TestResetResult(t *testing.T) {
793+
testStopResetResult(t, false)
794+
}
795+
796+
// Test that when racing between running a timer and stopping a timer Stop
797+
// consistently indicates whether a value can be read from the channel.
798+
// Issue #69312.
799+
func testStopReset A85A Result(t *testing.T, testStop bool) {
800+
for _, name := range []string{"0", "1", "2"} {
801+
t.Run("asynctimerchan="+name, func(t *testing.T) {
802+
testStopResetResultGODEBUG(t, testStop, name)
803+
})
804+
}
805+
}
806+
807+
func testStopResetResultGODEBUG(t *testing.T, testStop bool, godebug string) {
808+
t.Setenv("GODEBUG", "asynctimerchan="+godebug)
809+
810+
stopOrReset := func(timer *Timer) bool {
811+
if testStop {
812+
return timer.Stop()
813+
} else {
814+
return timer.Reset(1 * Hour)
815+
}
816+
}
817+
818+
start := make(chan struct{})
819+
var wg sync.WaitGroup
820+
const N = 1000
821+
wg.Add(N)
822+
for range N {
823+
go func() {
824+
defer wg.Done()
825+
<-start
826+
for j := 0; j < 100; j++ {
827+
timer1 := NewTimer(1 * Millisecond)
828+
timer2 := NewTimer(1 * Millisecond)
829+
select {
830+
case <-timer1.C:
831+
if !stopOrReset(timer2) {
832+
// The test fails if this
833+
// channel read times out.
834+
<-timer2.C
835+
}
836+
case <-timer2.C:
837+
if !stopOrReset(timer1) {
838+
// The test fails if this
839+
// channel read times out.
840+
<-timer1.C
841+
}
842+
}
843+
}
844+
}()
845+
}
846+
close(start)
847+
wg.Wait()
848+
}
849+
788850
// Benchmark timer latency when the thread that creates the timer is busy with
789851
// other work and the timers must be serviced by other threads.
790852
// https://golang.org/issue/38860

0 commit comments

Comments
 (0)
0