From bc5de81e70f9572d7b1d63040c95b999dac57f50 Mon Sep 17 00:00:00 2001
From: Michael Fraenkel <michael.fraenkel@gmail.com>
Date: Mon, 12 Apr 2021 18:34:04 -0600
Subject: [PATCH] testing: remove data races so that parallel benchmarks can
 safely call .Fatal* and .Skip*

Protects the usages of (*common).finished with locks
to prevent data races, thus allowing benchmarks to safely invoke
.Fatal* and .Skip* concurrently.

Fixes #45526

Change-Id: I2b4846f525c426d6c7d3418f8f6c86446adbf986
Reviewed-on: https://go-review.googlesource.com/c/go/+/309572
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Tobias Klauser <tobias.klauser@gmail.com>
---
 src/testing/benchmark.go      |  7 +++++--
 src/testing/benchmark_test.go | 24 ++++++++++++++++++++++++
 src/testing/testing.go        | 26 ++++++++++++++++----------
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index a8f75e97123..15b4426c5a5 100644
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -238,12 +238,15 @@ func (b *B) run1() bool {
 	}
 	// Only print the output if we know we are not going to proceed.
 	// Otherwise it is printed in processBench.
-	if atomic.LoadInt32(&b.hasSub) != 0 || b.finished {
+	b.mu.RLock()
+	finished := b.finished
+	b.mu.RUnlock()
+	if atomic.LoadInt32(&b.hasSub) != 0 || finished {
 		tag := "BENCH"
 		if b.skipped {
 			tag = "SKIP"
 		}
-		if b.chatty != nil && (len(b.output) > 0 || b.finished) {
+		if b.chatty != nil && (len(b.output) > 0 || finished) {
 			b.trimOutput()
 			fmt.Fprintf(b.w, "--- %s: %s\n%s", tag, b.name, b.output)
 		}
diff --git a/src/testing/benchmark_test.go b/src/testing/benchmark_test.go
index 4c1cbd19335..3b1dc8275b5 100644
--- a/src/testing/benchmark_test.go
+++ b/src/testing/benchmark_test.go
@@ -102,6 +102,30 @@ func TestRunParallelFail(t *testing.T) {
 	})
 }
 
+func TestRunParallelFatal(t *testing.T) {
+	testing.Benchmark(func(b *testing.B) {
+		b.RunParallel(func(pb *testing.PB) {
+			for pb.Next() {
+				if b.N > 1 {
+					b.Fatal("error")
+				}
+			}
+		})
+	})
+}
+
+func TestRunParallelSkipNow(t *testing.T) {
+	testing.Benchmark(func(b *testing.B) {
+		b.RunParallel(func(pb *testing.PB) {
+			for pb.Next() {
+				if b.N > 1 {
+					b.SkipNow()
+				}
+			}
+		})
+	})
+}
+
 func ExampleB_RunParallel() {
 	// Parallel benchmark for text/template.Template.Execute on a single object.
 	testing.Benchmark(func(b *testing.B) {
diff --git a/src/testing/testing.go b/src/testing/testing.go
index 851b118df4f..2146195956f 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -395,10 +395,10 @@ type common struct {
 	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.
 
 	chatty     *chattyPrinter // A copy of chattyPrinter, if the chatty flag is set.
 	bench      bool           // Whether the current test is a benchmark.
-	finished   bool           // Test function has completed.
 	hasSub     int32          // Written atomically.
 	raceErrors int            // Number of races detected during test.
 	runner     string         // Function name of tRunner running the test.
@@ -738,7 +738,9 @@ func (c *common) FailNow() {
 	// it would run on a test failure. Because we send on c.signal during
 	// a top-of-stack deferred function now, we know that the send
 	// only happens after any other stacked defers have completed.
+	c.mu.Lock()
 	c.finished = true
+	c.mu.Unlock()
 	runtime.Goexit()
 }
 
@@ -837,15 +839,11 @@ func (c *common) Skipf(format string, args ...interface{}) {
 // other goroutines created during the test. Calling SkipNow does not stop
 // those other goroutines.
 func (c *common) SkipNow() {
-	c.skip()
-	c.finished = true
-	runtime.Goexit()
-}
-
-func (c *common) skip() {
 	c.mu.Lock()
-	defer c.mu.Unlock()
 	c.skipped = true
+	c.finished = true
+	c.mu.Unlock()
+	runtime.Goexit()
 }
 
 // Skipped reports whether the test was skipped.
@@ -1138,10 +1136,16 @@ func tRunner(t *T, fn func(t *T)) {
 		err := recover()
 		signal := true
 
-		if !t.finished && err == nil {
+		t.mu.RLock()
+		finished := t.finished
+		t.mu.RUnlock()
+		if !finished && err == nil {
 			err = errNilPanicOrGoexit
 			for p := t.parent; p != nil; p = p.parent {
-				if p.finished {
+				p.mu.RLock()
+				finished = p.finished
+				p.mu.RUnlock()
+				if finished {
 					t.Errorf("%v: subtest may have called FailNow on a parent test", err)
 					err = nil
 					signal = false
@@ -1235,7 +1239,9 @@ func tRunner(t *T, fn func(t *T)) {
 	fn(t)
 
 	// code beyond here will not be executed when FailNow is invoked
+	t.mu.Lock()
 	t.finished = true
+	t.mu.Unlock()
 }
 
 // Run runs f as a subtest of t called name. It runs f in a separate goroutine