From: Bryan C. Mills <bcmills@google.com>
Date: Wed, 12 Apr 2023 14:45:44 +0000 (+0000)
Subject: net/http: only report the first leak of each test run
X-Git-Tag: go1.21rc1~934
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=b7428b7c6de6c7498425fe5374f22b7d3da054f4;p=gostls13.git

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 1e83ca3c0a..ff56ef883d 100644
--- 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 88184bcf35..164b18287f 100644
--- 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) {