From 4953b87296f53c5e0c7c62a775f1c088d4212902 Mon Sep 17 00:00:00 2001
From: Russ Cox <rsc@golang.org>
Date: Thu, 12 Jan 2012 10:18:12 -0800
Subject: [PATCH] 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
---
 src/pkg/testing/benchmark.go |  8 +++++++-
 src/pkg/testing/testing.go   | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/src/pkg/testing/benchmark.go b/src/pkg/testing/benchmark.go
index 4ce637082c..0bf567b7c4 100644
--- 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 a61ac0ea0b..d75dac8f60 100644
--- 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
-- 
2.50.0