New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: add test helpers for measuring goroutine leaks #6705

Open
bradfitz opened this Issue Nov 1, 2013 · 12 comments

Comments

Projects
None yet
10 participants
@bradfitz
Member

bradfitz commented Nov 1, 2013

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

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 1:

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

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 2:

Labels changed: added repo-main.

@bradfitz bradfitz self-assigned this Dec 4, 2013

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc removed release-none labels Apr 10, 2015

@akalin-keybase

This comment has been minimized.

akalin-keybase commented Dec 14, 2015

@bradfitz You reference this bug from #12989 saying that you were working on it. Has anything come out of that? :)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 15, 2015

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.

gopherbot pushed a commit that referenced this issue Apr 28, 2016

os/exec: fix variable shadow, don't leak goroutine
Goroutine leak checking is still too tedious, so untested.

See #6705 which is my fault for forgetting to mail out.

Change-Id: I899fb311c9d4229ff1dbd3f54fe307805e17efee
Reviewed-on: https://go-review.googlesource.com/22581
Reviewed-by: Ahmed W. <oneofone@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@joeshaw

This comment has been minimized.

Contributor

joeshaw commented Apr 29, 2016

@fortytw2

This comment has been minimized.

fortytw2 commented Apr 29, 2016

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

This comment has been minimized.

andrewdeandrade commented Aug 19, 2016

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

This comment has been minimized.

Contributor

anacrolix commented Mar 21, 2017

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

This comment has been minimized.

akshayjshah commented Aug 10, 2018

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

This comment has been minimized.

Contributor

ianlancetaylor commented Aug 10, 2018

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

This comment has been minimized.

akshayjshah commented Aug 11, 2018

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.

😻 😻 Couldn't agree more. 😻 😻

@klauspost

This comment has been minimized.

Contributor

klauspost commented Aug 13, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment