runtime: fix interactions between synctest, race detector, and timers
When an AfterFunc executes in a synctest bubble, there is a series of
happens-before relationships:
1. The AfterFunc is created.
2. The AfterFunc goroutine executes.
3. The AfterFunc goroutine returns.
4. A subsequent synctest.Wait call returns.
We were failing to correctly establish the happens-before relationship
between the AfterFunc goroutine and the AfterFunc itself being created.
When an AfterFunc executes, the G running the timer temporarily switches
to the timer heap's racectx. It then calls time.goFunc, which starts a
new goroutine to execute the timer. time.goFunc relies on the new goroutine
inheriting the racectx of the G running the timer.
Normal, non-synctest timers, execute with m.curg == nil, which causes
new goroutines to inherit the g0 racectx. We were running synctest
timers with m.curg set (to the G executing synctest.Run), so the new
AfterFunc goroutine was created using m.curg's racectx. This resulted
in us not properly establishing the happens-before relationship between
AfterFunc being called and the AfterFunc goroutine starting.
Fix this by setting m.curg to nil while executing timers.
As one additional fix, when waking a blocked bubble, wake the root
goroutine rather than a goroutine blocked in Wait if there is a
timer that can fire.
Fixes #72750
Change-Id: I2b2d6b0f17f64649409adc93c2603f720494af89
Reviewed-on: https://go-review.googlesource.com/c/go/+/658595
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
diff --git a/src/internal/synctest/synctest_test.go b/src/internal/synctest/synctest_test.go
index 62acb42..010679b 100644
--- a/src/internal/synctest/synctest_test.go
+++ b/src/internal/synctest/synctest_test.go
@@ -455,28 +455,89 @@
}
func TestHappensBefore(t *testing.T) {
- var v int
+ // Use two parallel goroutines accessing different vars to ensure that
+ // we correctly account for multiple goroutines in the bubble.
+ var v1 int
+ var v2 int
synctest.Run(func() {
+ v1++ // 1
+ v2++ // 1
+
+ // Wait returns after these goroutines exit.
go func() {
- v++ // 1
+ v1++ // 2
}()
- // Wait creates a happens-before relationship on the above goroutine exiting.
+ go func() {
+ v2++ // 2
+ }()
synctest.Wait()
+
+ v1++ // 3
+ v2++ // 3
+
+ // Wait returns after these goroutines block.
+ ch1 := make(chan struct{})
go func() {
- v++ // 2
+ v1++ // 4
+ <-ch1
+ }()
+ go func() {
+ v2++ // 4
+ <-ch1
+ }()
+ synctest.Wait()
+
+ v1++ // 5
+ v2++ // 5
+ close(ch1)
+
+ // Wait returns after these timers run.
+ time.AfterFunc(0, func() {
+ v1++ // 6
+ })
+ time.AfterFunc(0, func() {
+ v2++ // 6
+ })
+ synctest.Wait()
+
+ v1++ // 7
+ v2++ // 7
+
+ // Wait returns after these timer goroutines block.
+ ch2 := make(chan struct{})
+ time.AfterFunc(0, func() {
+ v1++ // 8
+ <-ch2
+ })
+ time.AfterFunc(0, func() {
+ v2++ // 8
+ <-ch2
+ })
+ synctest.Wait()
+
+ v1++ // 9
+ v2++ // 9
+ close(ch2)
+ })
+ // This Run happens after the previous Run returns.
+ synctest.Run(func() {
+ go func() {
+ go func() {
+ v1++ // 10
+ }()
+ }()
+ go func() {
+ go func() {
+ v2++ // 10
+ }()
}()
})
- // Run exiting creates a happens-before relationship on goroutines started in the bubble.
- synctest.Run(func() {
- v++ // 3
- // There is a happens-before relationship between the time.AfterFunc call,
- // and the func running.
- time.AfterFunc(0, func() {
- v++ // 4
- })
- })
- if got, want := v, 4; got != want {
- t.Errorf("v = %v, want %v", got, want)
+ // These tests happen after Run returns.
+ if got, want := v1, 10; got != want {
+ t.Errorf("v1 = %v, want %v", got, want)
+ }
+ if got, want := v2, 10; got != want {
+ t.Errorf("v2 = %v, want %v", got, want)
}
}
diff --git a/src/runtime/race/testdata/synctest_test.go b/src/runtime/race/testdata/synctest_test.go
new file mode 100644
index 0000000..dfbd568
--- /dev/null
+++ b/src/runtime/race/testdata/synctest_test.go
@@ -0,0 +1,97 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package race_test
+
+import (
+ "internal/synctest"
+ "testing"
+ "time"
+)
+
+func TestRaceSynctestGoroutinesExit(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ f := func() {
+ x = 1
+ }
+ go f()
+ go f()
+ })
+}
+
+func TestNoRaceSynctestGoroutinesExit(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ f := func() {
+ x = 1
+ }
+ go f()
+ synctest.Wait()
+ go f()
+ })
+}
+
+func TestRaceSynctestGoroutinesRecv(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ ch := make(chan struct{})
+ f := func() {
+ x = 1
+ <-ch
+ }
+ go f()
+ go f()
+ close(ch)
+ })
+}
+
+func TestRaceSynctestGoroutinesUnblocked(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ ch := make(chan struct{})
+ f := func() {
+ <-ch
+ x = 1
+ }
+ go f()
+ go f()
+ close(ch)
+ })
+}
+
+func TestRaceSynctestGoroutinesSleep(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ go func() {
+ time.Sleep(1 * time.Second)
+ x = 1
+ time.Sleep(2 * time.Second)
+ }()
+ go func() {
+ time.Sleep(2 * time.Second)
+ x = 1
+ time.Sleep(1 * time.Second)
+ }()
+ time.Sleep(5 * time.Second)
+ })
+}
+
+func TestRaceSynctestTimers(t *testing.T) {
+ synctest.Run(func() {
+ x := 0
+ _ = x
+ f := func() {
+ x = 1
+ }
+ time.AfterFunc(1*time.Second, f)
+ time.AfterFunc(2*time.Second, f)
+ time.Sleep(5 * time.Second)
+ })
+}
diff --git a/src/runtime/synctest.go b/src/runtime/synctest.go
index 65bb15d..f2ac6ab 100644
--- a/src/runtime/synctest.go
+++ b/src/runtime/synctest.go
@@ -79,6 +79,8 @@
} else {
sg.running--
if raceenabled && newval != _Gdead {
+ // Record that this goroutine parking happens before
+ // any subsequent Wait.
racereleasemergeg(gp, sg.raceaddr())
}
}
@@ -133,6 +135,11 @@
// Two wakes happening at the same time leads to very confusing failure modes,
// so we take steps to avoid it happening.
sg.active++
+ next := sg.timers.wakeTime()
+ if next > 0 && next <= sg.now {
+ // A timer is scheduled to fire. Wake the root goroutine to handle it.
+ return sg.root
+ }
if gp := sg.waiter; gp != nil {
// A goroutine is blocked in Wait. Wake it.
return gp
@@ -181,14 +188,14 @@
lock(&sg.mu)
sg.active++
for {
- if raceenabled {
- // Establish a happens-before relationship between a timer being created,
- // and the timer running.
- raceacquireg(gp, gp.syncGroup.raceaddr())
- }
unlock(&sg.mu)
systemstack(func() {
+ // Clear gp.m.curg while running timers,
+ // so timer goroutines inherit their child race context from g0.
+ curg := gp.m.curg
+ gp.m.curg = nil
gp.syncGroup.timers.check(gp.syncGroup.now)
+ gp.m.curg = curg
})
gopark(synctestidle_c, nil, waitReasonSynctestRun, traceBlockSynctest, 0)
lock(&sg.mu)
diff --git a/src/runtime/time.go b/src/runtime/time.go
index 3ece161..d27503e 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -1080,7 +1080,13 @@
// Note that we are running on a system stack,
// so there is no chance of getg().m being reassigned
// out from under us while this function executes.
- tsLocal := &getg().m.p.ptr().timers
+ gp := getg()
+ var tsLocal *timers
+ if t.ts == nil || t.ts.syncGroup == nil {
+ tsLocal = &gp.m.p.ptr().timers
+ } else {
+ tsLocal = &t.ts.syncGroup.timers
+ }
if tsLocal.raceCtx == 0 {
tsLocal.raceCtx = racegostart(abi.FuncPCABIInternal((*timers).run) + sys.PCQuantum)
}
@@ -1132,7 +1138,11 @@
if gp.racectx != 0 {
throw("unexpected racectx")
}
- gp.racectx = gp.m.p.ptr().timers.raceCtx
+ if ts == nil || ts.syncGroup == nil {
+ gp.racectx = gp.m.p.ptr().timers.raceCtx
+ } else {
+ gp.racectx = ts.syncGroup.timers.raceCtx
+ }
}
if ts != nil {
@@ -1193,6 +1203,11 @@
if ts != nil && ts.syncGroup != nil {
gp := getg()
ts.syncGroup.changegstatus(gp, _Grunning, _Gdead)
+ if raceenabled {
+ // Establish a happens-before between this timer event and
+ // the next synctest.Wait call.
+ racereleasemergeg(gp, ts.syncGroup.raceaddr())
+ }
gp.syncGroup = nil
}
