testing: fix racy t.done usage by adding a separate race-canary field

Author: Groxx

src/testing/fuzz.go

@@ -710,6 +710,7 @@ func fRunner(f *F, fn func(*F)) {
 
 		// Report after all subtests have finished.
 		f.report()
+		f.doneRaceCanary = true
 		f.done = true
 		f.setRan()
 	}()

src/testing/testing.go

@@ -593,20 +593,21 @@ const maxStackLen = 50
 // common holds the elements common between T and B and
 // captures common methods such as Errorf.
 type common struct {
-	mu          sync.RWMutex         // guards this group of fields
-	output      []byte               // Output generated by test or benchmark.
-	w           io.Writer            // For flushToParent.
-	ran         bool                 // Test or benchmark (or one of its subtests) was executed.
-	failed      bool                 // Test or benchmark has failed.
-	skipped     bool                 // Test or benchmark has been skipped.
-	done        bool                 // Test is finished and all subtests have completed.
-	helperPCs   map[uintptr]struct{} // functions to be skipped when writing file/line info
-	helperNames map[string]struct{}  // helperPCs converted to function names
-	cleanups    []func()             // optional functions to be called at the end of the test
-	cleanupName string               // Name of the cleanup function.
-	cleanupPc   []uintptr            // The stack trace at the point where Cleanup was called.
-	finished    bool                 // Test function has completed.
-	inFuzzFn    bool                 // Whether the fuzz target, if this is one, is running.
+	mu             sync.RWMutex         // guards this group of fields
+	output         []byte               // Output generated by test or benchmark.
+	w              io.Writer            // For flushToParent.
+	ran            bool                 // Test or benchmark (or one of its subtests) was executed.
+	failed         bool                 // Test or benchmark has failed.
+	skipped        bool                 // Test or benchmark has been skipped.
+	done           bool                 // Test is finished and all subtests have completed.
+	doneRaceCanary bool                 // Shadows all interactions with done except test-completion to trigger a race on misbehavior.
+	helperPCs      map[uintptr]struct{} // functions to be skipped when writing file/line info
+	helperNames    map[string]struct{}  // helperPCs converted to function names
+	cleanups       []func()             // optional functions to be called at the end of the test
+	cleanupName    string               // Name of the cleanup function.
+	cleanupPc      []uintptr            // The stack trace at the point where Cleanup was called.
+	finished       bool                 // Test function has completed.
+	inFuzzFn       bool                 // Whether the fuzz target, if this is one, is running.
 
 	chatty         *chattyPrinter // A copy of chattyPrinter, if the chatty flag is set.
 	bench          bool           // Whether the current test is a benchmark.
@@ -949,6 +950,7 @@ func (c *common) Fail() {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	// c.done needs to be locked to synchronize checks to c.done in parent tests.
+	_ = c.doneRaceCanary
 	if c.done {
 		panic("Fail in goroutine after " + c.name + " has completed")
 	}
@@ -960,6 +962,7 @@ func (c *common) Failed() bool {
 	c.mu.RLock()
 	defer c.mu.RUnlock()
 
+	_ = c.doneRaceCanary
 	if !c.done && int64(race.Errors()) > c.lastRaceErrors.Load() {
 		c.mu.RUnlock()
 		c.checkRaces()
@@ -1015,12 +1018,14 @@ func (c *common) log(s string) {
 func (c *common) logDepth(s string, depth int) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
+	_ = c.doneRaceCanary
 	if c.done {
 		// This test has already finished. Try and log this message
 		// with our parent. If we don't have a parent, panic.
 		for parent := c.parent; parent != nil; parent = parent.parent {
 			parent.mu.Lock()
 			defer parent.mu.Unlock()
+			_ = parent.doneRaceCanary
 			if !parent.done {
 				parent.output = append(parent.output, parent.decorate(s, depth+1)...)
 				return
@@ -1672,9 +1677,13 @@ func tRunner(t *T, fn func(t *T)) {
 		}
 		t.report() // Report after all subtests have finished.
 
-		// Do not lock t.done to allow race detector to detect race in case
-		// the user does not appropriately synchronize a goroutine.
+		// Do not lock around t.done's race canary to allow race detector to detect
+		// cases where the user does not appropriately synchronize a goroutine...
+		t.doneRaceCanary = true
+		t.mu.Lock()
+		// ...but do lock around t.done so after-done behavior is consistent.
 		t.done = true
+		t.mu.Unlock()
 		if t.parent != nil && !t.hasSub.Load() {
 			t.setRan()
 		}