Go: testing: add test helpers for measuring goroutine leaks

[bradfitz]

It's common for tests to check for goroutine leaks, but the testing package or standard
library doesn't make this easy.

These tests end up verbose and often frail.

We should provide something.

[rsc]

Comment 1:

_Labels changed: added release-none, removed go1.3maybe._

[rsc]

Comment 2:

_Labels changed: added repo-main._

[akalin-keybase]

@bradfitz You reference this bug from https://github.com/golang/go/issues/12989 saying that you were working on it. Has anything come out of that? :)

[bradfitz]

I do have a package partially written for this, but I never got happy enough to send it out and found more important things to do. I think I'll put it somewhere outside of the standard library first. I'll update this bug if/when I do.

[joeshaw]

Related: https://github.com/fortytw2/leaktest

[fortytw2]

That package still needs a bunch of tweaking/testing done to it -> I could send a CL that incorporates it into testing once it's more complete though. Had no clue this was an open issue 👍

[andrewdeandrade]

FWIW, we use example code like the following inside of any package where we want leaks testing:

package foobar

import (
    "fmt"
    "os"
    "testing"

    "github.com/uber/tchannel-go/testutils/goroutines"
)

func TestMain(m *testing.M) {
    exitCode := m.Run()
    if err := goroutines.IdentifyLeaks(&goroutines.VerifyOpts{
        Excludes: []string{
            // Raven: https://github.com/getsentry/raven-go/issues/90
            "getsentry/raven-go",
        },
    }); err != nil && exitCode == 0 {
        fmt.Fprintf(os.Stderr, "Found goroutine leaks on successful test run: %v", err)
        exitCode = 1
    }
    os.Exit(exitCode)
}

We haven't yet pulled this out as a separate library, but here's the source for reference: https://github.com/uber/tchannel-go/tree/dev/testutils/goroutines

[anacrolix]

I have been using https://godoc.org/github.com/anacrolix/missinggo/leaktest on a per test case basis in https://github.com/anacrolix/utp and https://github.com/anacrolix/ffprobe. It's rough, but does the job.

[akshayjshah]

Is the Go team willing to evaluate concrete proposals for something like this? Since @andrewdeandrade last commented, we've started using go.uber.org/goleak.VerifyTestMain in much of our internal code. It works pretty well, but has a number of shortcomings:

• Each package must opt into leak detection for its tests by implementing TestMain (or each test can use VerifyNoLeaks).
• There's no good equivalent of t.Skip, letting individual tests indicate that there's a known goroutine leak (but that additional leaks should still be caught).

It'd be really nice to have a more-capable goroutine leak detector built into the standard library's testing package. As a straw man, we could add a -leak flag to go test to opt into leak detection, and add a method to *testing.T and *testing.M to let tests indicate that there's a known leak:

// Leaks indicates that the test expects to leak N goroutines with the named function
// at the top of the stack. Setting N to zero allows the test to leak any number 
// of goroutines.
//
// This has no effect unless the goroutine leak detector is enabled.
func (t *testing.T) Leaks(fullyQualifiedFunctionName string, N int) {
  ...some implementation...
}

If the Go team is open to something like this, I'm happy to flesh out a full proposal or open a CL for feedback.

[ianlancetaylor]

I don't think that adding a -leak flag is any better than each test enabling leak detection itself. In fact, in some ways it's worse: now tests that are specifically intended to check for goroutine leaks will be useless unless the user remembers to pass the -leak flag.

So I think the first question should be whether we can get away with checking tests for leaking goroutines by default.

[akshayjshah]

I don't think that adding a -leak flag is any better than each test enabling leak detection itself. In fact, in some ways it's worse: now tests that are specifically intended to check for goroutine leaks will be useless unless the user remembers to pass the -leak flag.

In some ways, I agree - it's a bit like race detection today. Packages that I'm familiar with often include tests whose only purpose is to trigger any lurking data races, and those tests serve little purpose when executed without the -race flag. On the other hand, a flag seems nice because it lets the project owner's build/CI script set the default behavior for tests, rather than relying on code review to enforce a no-leaks norm.

So I think the first question should be whether we can get away with checking tests for leaking goroutines by default.

:heart_eyes_cat: :heart_eyes_cat: Couldn't agree more. :heart_eyes_cat: :heart_eyes_cat:

[klauspost]

So I think the first question should be whether we can get away with checking tests for leaking goroutines by default.

It would conflict with t.Parallel() unless there is more magic than I expect. I guess a panic wouldn't be unreasonable in the case that both are requested.

From a practical perspective, I would think that adding a t.TestGoroutinesExit() (or whatever it would be called) would be a good practical step one. Maybe adding it to testing.M as well would make it easy to enable for an entire package.

[eliben]

Another leak detector module that was released recently: https://github.com/uber-go/goleak