testing: fix defer race

In a test that does

        func TestFoo(t *testing.T) {
                defer cleanup()
                t.Fatal("oops")
        }

it can be important that cleanup run as the test fails.
The old code did this in Fatal:

        t.signal <- t
        runtime.Goexit()

The runtime.Goexit would run the deferred cleanup
but the send on t.signal would cause the main test loop
to move on and possibly even exit the program before
the runtime.Goexit got a chance to run.

This CL changes tRunner (the top stack frame of a test
goroutine) to send on t.signal as part of a function
deferred by the top stack frame.  This delays the send
on t.signal until after runtime.Goexit has run functions
deferred by the test itself.

For the above TestFoo, this CL guarantees that cleanup
will run before the test binary exits.

This is particularly important when cleanup is doing
externally visible work, like removing temporary files
or unmounting file systems.

R=golang-dev, r
CC=golang-dev
https://golang.org/cl/5532078

diff --git a/src/pkg/testing/benchmark.go b/src/pkg/testing/benchmark.go
index 4ce637082ca660d3441ab484b0d02359849b6310..0bf567b7c4d898fa6e159a15e1c198a6a7a12e64 100644 (file)
--- a/src/pkg/testing/benchmark.go
+++ b/src/pkg/testing/benchmark.go
@@ -142,6 +142,13 @@ func (b *B) run() BenchmarkResult {
 func (b *B) launch() {
        // Run the benchmark for a single iteration in case it's expensive.
        n := 1
+
+       // Signal that we're done whether we return normally
+       // or by FailNow's runtime.Goexit.
+       defer func() {
+               b.signal <- b
+       }()
+
        b.runN(n)
        // Run the benchmark for at least the specified amount of time.
        d := time.Duration(*benchTime * float64(time.Second))
@@ -162,7 +169,6 @@ func (b *B) launch() {
                b.runN(n)
        }
        b.result = BenchmarkResult{b.N, b.duration, b.bytes}
-       b.signal <- b
 }
 
 // The results of a benchmark run.

diff --git a/src/pkg/testing/testing.go b/src/pkg/testing/testing.go
index a61ac0ea0b2ba28595d11de800e51b5f6b37ca76..d75dac8f60566cf3e38332099fc8c6a9ef357036 100644 (file)
--- a/src/pkg/testing/testing.go
+++ b/src/pkg/testing/testing.go
@@ -136,9 +136,27 @@ func (c *common) Failed() bool { return c.failed }
 // FailNow marks the function as having failed and stops its execution.
 // Execution will continue at the next Test.
 func (c *common) FailNow() {
-       c.duration = time.Now().Sub(c.start)
        c.Fail()
-       c.signal <- c.self
+
+       // Calling runtime.Goexit will exit the goroutine, which
+       // will run the deferred functions in this goroutine,
+       // which will eventually run the deferred lines in tRunner,
+       // which will signal to the test loop that this test is done.
+       //
+       // A previous version of this code said:
+       //
+       //      c.duration = ...
+       //      c.signal <- c.self
+       //      runtime.Goexit()
+       //
+       // This previous version duplicated code (those lines are in
+       // tRunner no matter what), but worse the goroutine teardown
+       // implicit in runtime.Goexit was not guaranteed to complete
+       // before the test exited.  If a test deferred an important cleanup
+       // function (like removing temporary files), there was no guarantee
+       // 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.
        runtime.Goexit()
 }
 
@@ -195,9 +213,17 @@ type InternalTest struct {
 
 func tRunner(t *T, test *InternalTest) {
        t.start = time.Now()
+
+       // When this goroutine is done, either because test.F(t)
+       // returned normally or because a test failure triggered 
+       // a call to runtime.Goexit, record the duration and send
+       // a signal saying that the test is done.
+       defer func() {
+               t.duration = time.Now().Sub(t.start)
+               t.signal <- t
+       }()
+
        test.F(t)
-       t.duration = time.Now().Sub(t.start)
-       t.signal <- t
 }
 
 // An internal function but exported because it is cross-package; part of the implementation