Skip to content

Commit

Permalink
net/http: only report the first leak of each test run
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
  • Loading branch information
Bryan C. Mills committed Apr 12, 2023
1 parent 4b154e5 commit b7428b7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 3 deletions.
20 changes: 20 additions & 0 deletions src/net/http/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -132,6 +151,7 @@ func afterTest(t testing.TB) {
}
}
if bad == "" {
leakReported = false
return
}
// Bad stuff found, but goroutines might just still be
Expand Down
3 changes: 0 additions & 3 deletions src/net/http/serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit b7428b7

Please sign in to comment.