testing: make parallel t.Run safe again
Fixes #18603.
Change-Id: I5760c0a9f862200b7e943058a672eb559ac1b9d9
Reviewed-on: https://go-review.googlesource.com/35354
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/src/testing/benchmark.go b/src/testing/benchmark.go
index c033ce5..bcebb41 100644
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -219,7 +219,7 @@
}
// Only print the output if we know we are not going to proceed.
// Otherwise it is printed in processBench.
- if b.hasSub || b.finished {
+ if atomic.LoadInt32(&b.hasSub) != 0 || b.finished {
tag := "BENCH"
if b.skipped {
tag = "SKIP"
@@ -460,10 +460,13 @@
//
// A subbenchmark is like any other benchmark. A benchmark that calls Run at
// least once will not be measured itself and will be called once with N=1.
+//
+// Run may be called simultaneously from multiple goroutines, but all such
+// calls must happen before the outer benchmark function for b returns.
func (b *B) Run(name string, f func(b *B)) bool {
// Since b has subbenchmarks, we will no longer run it as a benchmark itself.
// Release the lock and acquire it on exit to ensure locks stay paired.
- b.hasSub = true
+ atomic.StoreInt32(&b.hasSub, 1)
benchmarkLock.Unlock()
defer benchmarkLock.Lock()
diff --git a/src/testing/sub_test.go b/src/testing/sub_test.go
index 8d5d920..bb7b3e0 100644
--- a/src/testing/sub_test.go
+++ b/src/testing/sub_test.go
@@ -6,6 +6,7 @@
import (
"bytes"
+ "fmt"
"regexp"
"strings"
"sync/atomic"
@@ -515,3 +516,19 @@
Benchmark(func(b *B) { b.Error("do not print this output") })
Benchmark(func(b *B) {})
}
+
+func TestParallelSub(t *T) {
+ c := make(chan int)
+ block := make(chan int)
+ for i := 0; i < 10; i++ {
+ go func(i int) {
+ <-block
+ t.Run(fmt.Sprint(i), func(t *T) {})
+ c <- 1
+ }(i)
+ }
+ close(block)
+ for i := 0; i < 10; i++ {
+ <-c
+ }
+}
diff --git a/src/testing/testing.go b/src/testing/testing.go
index c972b27..ddbdc25 100644
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -216,6 +216,7 @@
"strconv"
"strings"
"sync"
+ "sync/atomic"
"time"
)
@@ -267,8 +268,8 @@
skipped bool // Test of benchmark has been skipped.
finished bool // Test function has completed.
done bool // Test is finished and all subtests have completed.
- hasSub bool
- raceErrors int // number of races detected during test
+ hasSub int32 // written atomically
+ raceErrors int // number of races detected during test
parent *common
level int // Nesting depth of test or benchmark.
@@ -645,7 +646,7 @@
// Do not lock t.done to allow race detector to detect race in case
// the user does not appropriately synchronizes a goroutine.
t.done = true
- if t.parent != nil && !t.hasSub {
+ if t.parent != nil && atomic.LoadInt32(&t.hasSub) == 0 {
t.setRan()
}
t.signal <- true
@@ -659,8 +660,11 @@
// Run runs f as a subtest of t called name. It reports whether f succeeded.
// Run will block until all its parallel subtests have completed.
+//
+// Run may be called simultaneously from multiple goroutines, but all such
+// calls must happen before the outer test function for t returns.
func (t *T) Run(name string, f func(t *T)) bool {
- t.hasSub = true
+ atomic.StoreInt32(&t.hasSub, 1)
testName, ok := t.context.match.fullName(&t.common, name)
if !ok {
return true
