uber-go / goleak

Goroutine leak detector
MIT License
4.48k stars 148 forks source link

add teardown option and create a unit test for it #106

Closed ELchem closed 1 year ago

ELchem commented 1 year ago

It would be useful to be able to run a teardown function in VerifyTestMain that could stop any goroutines that would otherwise have to be stopped in each testcase. For example, in the go-cache library, there is a janitor function that must be stopped explicitly by the user but if go-cache is used implicitly in a test package, then every testcase needs to stop the janitor function. With a teardown function, the hanging goroutine could be cancelled after all tests are run.

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


EL seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

JacobOaks commented 1 year ago

Hey, thanks for this PR.

Can you give us an example as to why this needs to be a part of goleak? From my understanding, test cleanup can be handled with well with https://pkg.go.dev/testing#T.Cleanup.

ELchem commented 1 year ago

Can you give us an example as to why this needs to be a part of goleak? From my understanding, test cleanup can be handled with well with https://pkg.go.dev/testing#T.Cleanup.

Thank you for the response and making me aware of https://pkg.go.dev/testing#T.Cleanup. From my understanding, T.Cleanup is defined and called in each test. I was interested in creating a cleanup/teardown function that only needs to be called once after all tests have run.

JacobOaks commented 1 year ago

I'm curious why this should be a part of goleak though? There are already other ways to do a cleanup after all tests have been run, such as with a custom TestMain, or utilizing something like stretchr/testify/suite which has something like this.

ELchem commented 1 year ago

My concern is that if I want to use goleak in TestMain, then I have no way of calling a cleanup function between when goleak runs the unit tests and when it checks for leaks. My thought here is that a general resource used by all unit tests (such as a pool of goroutines) could be closed at once after all the tests have been run by passing in a teardown function.

sywhang commented 1 year ago

@ELchem that sounds like a very specific use case, is there an example of where you're doing such a thing? In general, if there is a goroutine pool or something that you're using that needs to be cleaned up i would believe such a thing exists in the product code, not the test-specific code.

For the specific use case you mentioned about the go-cache library, it looks like the stopJanitor function is called once Cache is garbage-collected (https://github.com/patrickmn/go-cache/blob/46f407853014144407b6c2ec7ccc76bf67958d93/cache.go#L1123), so you know for sure that this does not leak. You can opt that out of goleak check with goleak.IgnoreTopFunction.

At Uber we have a list of goroutines that is known to leak at test time and ignore them globally using IgnoreTopFunction.

abhinav commented 1 year ago

In addition to @sywhang and @JacobOaks's concerns, I just want to mention that a Teardown option has a high probability of confusing users because there's already a Cleanup option (which runs at the end, after all leaks have been searched for.

For your specific use case, @ELchem, you can ignore the known leaks as @sywhang suggested. Alternatively, you can wrap m.Run(). goleak.VerifyTestMain takes an interface, so you can do this:

// runFunc implements goleak.TestingM around a function reference.
type runFunc func() int

func (f runFunc) Run() int {
  return f()
}

func TestMain(m *testing.M) {
  cache := setUpCache()
  goleak.VerifyTestMain(
    runFunc(func() int {
      exitCode := m.Run()
      cache.Cleanup()
    }),
  )
}

It's a bit of boilerplate, but it helps address the fairly specific concern without confusing the terms in goleak itself.

ELchem commented 1 year ago

Thank you all, I will close this PR.