uber-go / goleak

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

default ignorelist entry for rules_go SIGTERM handler #119

Open malt3 opened 8 months ago

malt3 commented 8 months ago

rules_go added a SIGTERM handler that adds a leaking goroutine to test cases: https://github.com/bazelbuild/rules_go/pull/3749

goleak will come with a default ignorelist entry in the future as far as I understood @linzhp.

-- https://github.com/bazelbuild/rules_go/pull/3827#issuecomment-1892145580

Can this be added? Adding a manual ignore in every test is not very usable.

sywhang commented 8 months ago

This isn't being considered at the moment. Every stack we ignore by default comes at the cost, and most of users of goleak aren't on rules_go. AFAIK support of this feature isn't something that was discussed with goleak maintainers.

fmeum commented 8 months ago

Sorry for the confusion I caused with the statement quoted above, I think I misunderstood (and thus misrepresented) Lin's plans.

The special situation with rules_go is that we (have to) implement a custom test runner, but users have control over when they run goleak's checks and we can't cancel the goroutine in time.

I fully understand that goleak doesn't want to maintain a central ignore list. Unfortunately, since the test runner must stay lean and free of external dependencies, we also can't call any of the goleak ignore functions such as IgnoreAnyFunction.

@sywhang Do you see a way for goleak to provide "frameworks" such as the rules_go test runner with a way to have certain goroutines ignored by goleak without requiring a dependency on the module? For example, by accepting a list of function names via

sywhang commented 8 months ago

Hey @fmeum, thanks for providing the context around this.

A package-level variable seems like a reasonable solution that's lightweight enough we could consider adding. Seems like this could be useful for non rules_go users as well for cases where there's monorepo-like environments crossing multiple packages.

Something like: (naming tbd)

var DefaultIgnoreFunctionSet string // comma-separated list of functions that may leak.

Tagging other maintainers for additional thoughts: @r-hang @abhinav @prashantv

Separately, at Uber's Go Monorepo (where we do use rules_go to run tests), we have a file that specifies the list of known leaks - we hijack the rules_go TestMain template generation to inject that list of known leaks at build time and call IgnoreTopFunction on them.

fmeum commented 7 months ago

@malt3 Could you reopen this issue?

malt3 commented 7 months ago

Sorry! Was an automated close from the PR in another repo

fmeum commented 7 months ago

@sywhang Friendly ping, is this something you would accept contributions for in case all of you are busy?

sywhang commented 1 month ago

@fmeum sorry for the months of delay in response - #127 should fix this.

abhinav commented 1 month ago

Tagging other maintainers for additional thoughts: @r-hang @abhinav @prashantv

My apologies I completely missed this.