net/http: only report the first leak of each test run

We don't have a way to terminate the leaked goroutines, and we can't
wait forever for them to exit (or else we would risk timing out the
test and losing the log line describing what exactly leaked).
So we have reason to believe that they will remain leaked while we run
the next test, and we don't want the goroutines from the first leak to
generate a spurious error when the second test completes.

This also removes a racy Parallel call I added in CL 476036, which was
flagged by the race detector in the duplicate-suppression check.
(I hadn't considered the potential interaction with the leak checker.)

For #59526.
Updates #56421.

Change-Id: Ib1f759f102fb41ece114401680cd728343e58545
Reviewed-on: https://go-review.googlesource.com/c/go/+/483896
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>

diff --git a/src/net/http/main_test.go b/src/net/http/main_test.go
index 1e83ca3c0ac5caf0fca9f8631f3c9741784a6a1e..ff56ef883d834cd27d22e7372e8a9747a25cc72a 100644 (file)
--- a/src/net/http/main_test.go
+++ b/src/net/http/main_test.go
@@ -108,11 +108,30 @@ func runningBenchmarks() bool {
        return false
 }
 
+var leakReported bool
+
 func afterTest(t testing.TB) {
        http.DefaultTransport.(*http.Transport).CloseIdleConnections()
        if testing.Short() {
                return
        }
+       if leakReported {
+               // To avoid confusion, only report the first leak of each test run.
+               // After the first leak has been reported, we can't tell whether the leaked
+               // goroutines are a new leak from a subsequent test or just the same
+               // goroutines from the first leak still hanging around, and we may add a lot
+               // of latency waiting for them to exit at the end of each test.
+               return
+       }
+
+       // We shouldn't be running the leak check for parallel tests, because we might
+       // report the goroutines from a test that is still running as a leak from a
+       // completely separate test that has just finished. So we use non-atomic loads
+       // and stores for the leakReported variable, and store every time we start a
+       // leak check so that the race detector will flag concurrent leak checks as a
+       // race even if we don't detect any leaks.
+       leakReported = true
+
        var bad string
        badSubstring := map[string]string{
                ").readLoop(":  "a Transport",
@@ -132,6 +151,7 @@ func afterTest(t testing.TB) {
                        }
                }
                if bad == "" {
+                       leakReported = false
                        return
                }
                // Bad stuff found, but goroutines might just still be

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 88184bcf356d65e4a6a0a5bfc0bc5d93364e601e..164b18287f179ec6bee542bba0dfa9152b268905 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -5519,9 +5519,6 @@ func testServerShutdownStateNew(t *testing.T, mode testMode) {
        if testing.Short() {
                t.Skip("test takes 5-6 seconds; skipping in short mode")
        }
-       // The run helper runs the test in parallel only in short mode by default.
-       // Since this test has a very long latency, always run it in parallel.
-       t.Parallel()
 
        var connAccepted sync.WaitGroup
        ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {