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 c033ce5fecb1286965089adde2a4919ff2a4da84..bcebb418c42572e1c26aa3427926667c596adf5b 100644 (file)
--- a/src/testing/benchmark.go
+++ b/src/testing/benchmark.go
@@ -219,7 +219,7 @@ 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 b.hasSub || b.finished {
+       if atomic.LoadInt32(&b.hasSub) != 0 || b.finished {
                tag := "BENCH"
                if b.skipped {
                        tag = "SKIP"
@@ -460,10 +460,13 @@ func (ctx *benchContext) processBench(b *B) {
 //
 // 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 8d5d9206f03563633e457afceb37b035d861f2cc..bb7b3e09255ea167de3001248b55c83e20b21109 100644 (file)
--- a/src/testing/sub_test.go
+++ b/src/testing/sub_test.go
@@ -6,6 +6,7 @@ package testing
 
 import (
        "bytes"
+       "fmt"
        "regexp"
        "strings"
        "sync/atomic"
@@ -515,3 +516,19 @@ func TestBenchmarkOutput(t *T) {
        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 c972b2737f28d80692e2e8a18f27a7fc53350615..ddbdc25bf11d5d7d69d5443ec68b74cad413d12d 100644 (file)
--- a/src/testing/testing.go
+++ b/src/testing/testing.go
@@ -216,6 +216,7 @@ import (
        "strconv"
        "strings"
        "sync"
+       "sync/atomic"
        "time"
 )
 
@@ -267,8 +268,8 @@ type common struct {
        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 @@ func tRunner(t *T, fn func(t *T)) {
                // 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 @@ func tRunner(t *T, fn func(t *T)) {
 
 // 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