Support goroutine leak detection

[BooleanCat]

Hi there.

In a suite test I recently wrote , I added the following to the AfterSuite to detect if there were go routines being leaked at all:

var _ AfterSuite(func() {
  Eventually(leaks).ShouldNot(HaveOccurred())
})

func leaks() error {
  goleak.FindLeaks(goleak.IgnoreTopFunction("github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).registerForInterrupts"))
}

The goleak package being used is go.uber.org/goleak.

Seems to be working fine, but I'm aware that I'm relying on an internal structure of Ginkgo to ignore go routines spinning within the ginkgo framework.

Would you consider support for something like this natively within ginkgo? Maybe ginkgo --detectGoroutineLeaks.

cc @zknill

[blgm]

This looks like a really useful thing to test. Thank you for sharing it. I can see how relying on internal names within Ginkgo is messy.

The suggestion is that another Ginkgo flag could be added to trigger an optional behaviour. A disadvantage of this is that every Ginkgo user would have the complexity of depending on and downloading the go.uber.org/goleak code even if they are not using the functionality.

Have you considered publishing this as a stand-alone plugin - for instance as a Gomega matcher? This would allow it to be imported by people who want to use it, and not by those who want Ginkgo to be small and fast. It would be a way to experiment with the popularity of the approach without committing to supporting it, and without Ginkgo having to commit to a particular approach for detecting goroutine leaks.

A concern I have is that there's no consensus in the Go community on how to test for goroutine leaks. There are arguments for doing it this way, and there are arguments building it into go test. That's why I would prefer an experiment rather than a new feature. But I would be very interested to hear other thoughts and opinions on this.

[BooleanCat]

A disadvantage of this is that every Ginkgo user would have the complexity of depending on and downloading the go.uber.org/goleak code even if they are not using the functionality.

I wouldn't necessarily say that we must use Uber's implementation of it, I could write an implementation within Ginkgo and incur no additional dependencies.

[l0nax]

Any updates on this?

[onsi]

I'm going through the backlog of Ginkgo issues and am only just getting to this, sorry!

This would be a great feature, perhaps as a Gomega package that is Ginkgo and Gomega aware and filters out any non-Ginkgo coroutines to produce a leak report. I'll add it to my v2 backlog. @BooleanCat if you're still interested in a PR we can think through it together. Otherwise I can get to this when it eventually comes up in the backlog.

[BooleanCat]

Thanks for the pokes! I never got around to working on this and I'm unlikely to find time at least in the near future to work on it I'm afraid.

[onsi]

Sounds good - no worries!

[thediveo]

As I wondered about goroutines in some of my projects and to detect them in testing, a search for "gomega goroutine leak" teleported me here. Not only does this issue has the interesting goleaks link, but also a valuable discussion around it ... even as it has been dormant for quite some time.

Initially, I thought about wrapping Über's (yepp, I'm entitled to use proper umlauts) goleak for use with Gomega. However, I noticed several problems:

1. goleak already uses its own Eventually-like retry mechanism, so it should be used as Expect(goleak.Find()).To(HaveOccured()) ... well, it gets the job done, but it is rather gomicron than smooth gomega. Eventually(Goroutines).ShouldNot(HaveLeaked()) can be achieved by aliasing, but basically still works on error results instead of reasoning about goroutine lists.
2. unfortunately, because there wasn't any need before to do so, goleak does not export its stacks internal package for getting a list of "stacks" ... which are actually goroutine stacks and associated information.
3. reuse Gomega's try-it-till-it-makes-it Eventually mechanism that is controllable using Gomega's established API (.WithTimeout(...), yada yada yada...).

So I've started from scratch with some code I will soon make public on Github for further discussion. My approach at this time is, as @blgm suggested, a Gomega matcher extension experiment. A Goroutines() function returns a list (slice) of Goroutines in order to reason about them using a HaveLeaked(...) matcher.

In order to filter out (exclude?) certain goroutines from the list of leakers, how would you specify them to the HaveLeaked() matcher? Using an option mechanism like the original goleak package does? In the context of Gomega matchers, a generic option pattern might be inappropriate ... what do you here think?

Would a better and more natural design in the Gomega context to accept one or more elements as the HaveLeaked() matcher, where these elements can either be of type Goroutine (both scalars and slices?) Would a simple string be sensible that identifies the name of the topmost function in a Goroutine's backtrace?

Should we accept other matchers that reason on the name of the topmost function of the actual slice/list of Goroutines?

[thediveo]

@onsi @BooleanCat may I propose ... noleak, to complement Gomega with goroutine discovery and leak matchers?

Simplest form:

AfterEach(func() {
    // Notice: Goroutines, not: Goroutines()
    Eventually(Goroutines).ShouldNot(HaveLeaked())
})

Or adapting to background goroutine noise:

var snapshot []Goroutine

BeforeEach(func() {
    snapshot = Goroutines()
})

AfterEach(func() {
    Eventually(Goroutines).ShouldNot(HaveLeaked(snapshot))
})

The goroutine filtering to find leaked goroutines uses GomegaMatchers to reason about Goroutines. Comes with predefined goroutine matchers, as well as accepts shorthand notation for some common use cases (just passing snapshot is an example of such a shorthand, instead of IgnoringGoroutines(snapshot)).

Your feedback on this experiment is highly appreciated!

As for the license: I see no problems in changing it in case you want to integrate noleak into Gomega.

[onsi]

OMG this is a thing of beauty 🤩. I love the interface you've come up with! My only question is wether or not G should be exported? My sense is the intent is for it to be used internally by the various noleak matchers?

If you're game with changing the license I'd love to include this in Gomega - it could be a new package alongside gexec, gmeasure, etc. in the past I've shipped initial versions of such sub-packages as betas to get some feedback and have freedom to change the external interface before committing to a "GA" and we could do the same here.

We could stick with noleak or change to something like gleak to match the existing naming "convention" but I don't feel strongly. And I'd be happy to help however would be most helpful example updating the markdown docs, pulling the code in, etc.

[thediveo]

• G ... intentionally is exported as this allows users to reuse that convenience in case they need to write their own goroutine filter matchers. As a nice bonus, it still allows to use the shorthand variable name g for a goroutine description. 😉
• as I said, changing the license to Gomega's MIT-style license is no problem for me. It's an honor to me to get integrated into Gomega!
• changing to gleak to go with the g theme is fine.

I would like to create a new feature request "issue" with Gomega in order to track our further discussion over there and for creating a related PR; would that be okay with you?

But now, what am I to do with my new mascot...?

[onsi]

LOL that can go in the docs for sure :)

sounds good all around - feel free to open a new issue. thanks for contributing this!