From c0392e7e494c7e1fa7122df3cb5c1a30760ac5b4 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 17 Mar 2025 13:48:50 -0700 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt --- src/internal/synctest/synctest_test.go | 87 ++++++++++++++++--- src/runtime/race/testdata/synctest_test.go | 97 ++++++++++++++++++++++ src/runtime/synctest.go | 17 ++-- src/runtime/time.go | 19 ++++- 4 files changed, 200 insertions(+), 20 deletions(-) create mode 100644 src/runtime/race/testdata/synctest_test.go diff --git a/src/internal/synctest/synctest_test.go b/src/internal/synctest/synctest_test.go index 62acb42359..010679b070 100644 --- a/src/internal/synctest/synctest_test.go +++ b/src/internal/synctest/synctest_test.go @@ -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 index 0000000000..dfbd5682ca --- /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 65bb15dfbb..f2ac6ab5c7 100644 --- a/src/runtime/synctest.go +++ b/src/runtime/synctest.go @@ -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) diff --git a/src/runtime/time.go b/src/runtime/time.go index 3ece161cf4..d27503e4df 100644 --- a/src/runtime/time.go +++ b/src/runtime/time.go @@ -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 } -- 2.51.0