]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: fix interactions between synctest, race detector, and timers
authorDamien Neil <dneil@google.com>
Mon, 17 Mar 2025 20:48:50 +0000 (13:48 -0700)
committerGopher Robot <gobot@golang.org>
Fri, 21 Mar 2025 16:32:55 +0000 (09:32 -0700)
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>
src/internal/synctest/synctest_test.go
src/runtime/race/testdata/synctest_test.go [new file with mode: 0644]
src/runtime/synctest.go
src/runtime/time.go

index 62acb42359acff5ada64ee3596fdbaf3cb024605..010679b070f87a3167df8d6e0d689cc89d926213 100644 (file)
@@ -455,28 +455,89 @@ func TestWaitGroup(t *testing.T) {
 }
 
 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() {
+                       v1++ // 2
+               }()
                go func() {
-                       v++ // 1
+                       v2++ // 2
                }()
-               // Wait creates a happens-before relationship on the above goroutine exiting.
                synctest.Wait()
+
+               v1++ // 3
+               v2++ // 3
+
+               // Wait returns after these goroutines block.
+               ch1 := make(chan struct{})
                go func() {
-                       v++ // 2
+                       v1++ // 4
+                       <-ch1
                }()
-       })
-       // 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.
+               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() {
-                       v++ // 4
+                       v1++ // 8
+                       <-ch2
                })
+               time.AfterFunc(0, func() {
+                       v2++ // 8
+                       <-ch2
+               })
+               synctest.Wait()
+
+               v1++ // 9
+               v2++ // 9
+               close(ch2)
        })
-       if got, want := v, 4; got != want {
-               t.Errorf("v = %v, want %v", got, want)
+       // This Run happens after the previous Run returns.
+       synctest.Run(func() {
+               go func() {
+                       go func() {
+                               v1++ // 10
+                       }()
+               }()
+               go func() {
+                       go func() {
+                               v2++ // 10
+                       }()
+               }()
+       })
+       // 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 (file)
index 0000000..dfbd568
--- /dev/null
@@ -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)
+       })
+}
index 65bb15dfbb04554e5393b93b9d1051a2bb34508d..f2ac6ab5c755e1ff0f4c798adce6f12d1ce986ad 100644 (file)
@@ -79,6 +79,8 @@ func (sg *synctestGroup) changegstatus(gp *g, oldval, newval uint32) {
                } 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 @@ func (sg *synctestGroup) maybeWakeLocked() *g {
        // 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 @@ func synctestRun(f func()) {
        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)
index 3ece161cf47e5a6a880d068348a6dcb9a5dbc8b0..d27503e4df83febf36ea5fb5ee78eb9d8bc7db9f 100644 (file)
@@ -1080,7 +1080,13 @@ func (t *timer) unlockAndRun(now int64) {
                // 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 @@ func (t *timer) unlockAndRun(now int64) {
                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 @@ func (t *timer) unlockAndRun(now int64) {
        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
        }