Skip to content
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

default ignorelist entry for rules_go SIGTERM handler #119

Open
malt3 opened this issue Jan 15, 2024 · 8 comments · Fixed by edgelesssys/constellation#2824 · May be fixed by #127
Open

default ignorelist entry for rules_go SIGTERM handler #119

malt3 opened this issue Jan 15, 2024 · 8 comments · Fixed by edgelesssys/constellation#2824 · May be fixed by #127
Assignees

Comments

@malt3
Copy link

malt3 commented Jan 15, 2024

rules_go added a SIGTERM handler that adds a leaking goroutine to test cases: bazel-contrib/rules_go#3749

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

-- bazel-contrib/rules_go#3827 (comment)

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

@sywhang
Copy link
Contributor

sywhang commented Jan 15, 2024

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.

malt3 added a commit to edgelesssys/constellation that referenced this issue Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
malt3 added a commit to edgelesssys/constellation that referenced this issue Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
malt3 added a commit to edgelesssys/constellation that referenced this issue Jan 16, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
@fmeum
Copy link

fmeum commented Jan 16, 2024

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

  • an environment variable
  • a package-level variable that can be set at linktime (via the -X ldflag or x_defs in rules_go)
  • a naming convention (e.g. ignoring all stacks where the top function is called ...GoleakIgnoreLeaks

@sywhang
Copy link
Contributor

sywhang commented Jan 16, 2024

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.

malt3 added a commit to edgelesssys/constellation that referenced this issue Jan 22, 2024
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check.
Currently, the best known workaround is to ignore this goroutine.

uber-go/goleak#119
bazel-contrib/rules_go#3749
bazel-contrib/rules_go#3827 (comment)
@fmeum
Copy link

fmeum commented Jan 22, 2024

@malt3 Could you reopen this issue?

@malt3
Copy link
Author

malt3 commented Jan 22, 2024

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

@malt3 malt3 reopened this Jan 22, 2024
@fmeum
Copy link

fmeum commented Feb 7, 2024

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

@JacobOaks JacobOaks assigned JacobOaks and sywhang and unassigned JacobOaks Feb 20, 2024
sywhang added a commit to sywhang/goleak that referenced this issue Jul 24, 2024
This exposes a variable `DefaultIgnoreFunctionSet`, which can be used
to set a default list of functions to ignore for goleak.

Note that this mechanism is intended for library or tool authors who
cannot directly call Ignore* Options to set up how their consumers are
running the tests. They should be able to use linkname to set this
variable to some well-known list of goroutines that leak if they really
want to exempt their goroutines from getting continously flagged by
goleak.

Fixes uber-go#119.
sywhang added a commit to sywhang/goleak that referenced this issue Jul 24, 2024
This exposes a variable `DefaultIgnoreFunctionSet`, which can be used
to set a default list of functions to ignore for goleak.

Note that this mechanism is intended for library or tool authors who
cannot directly call Ignore* Options to set up how their consumers are
running the tests. They should be able to use linkname to set this
variable to some well-known list of goroutines that leak if they really
want to exempt their goroutines from getting continously flagged by
goleak.

Fixes uber-go#119.
@sywhang sywhang linked a pull request Jul 24, 2024 that will close this issue
@sywhang
Copy link
Contributor

sywhang commented Jul 24, 2024

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

@abhinav
Copy link
Collaborator

abhinav commented Jul 24, 2024

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

My apologies I completely missed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants