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

ginkgo v2 leaks goroutine #1364

Open
cyga opened this issue Mar 1, 2024 · 3 comments
Open

ginkgo v2 leaks goroutine #1364

cyga opened this issue Mar 1, 2024 · 3 comments

Comments

@cyga
Copy link

cyga commented Mar 1, 2024

How to reproduce:

cd /tmp
mkdir gokingoleak
echo 'package gokingoleak

import (
  "testing"

  "github.com/onsi/ginkgo/v2"
  "github.com/onsi/gomega"
  "go.uber.org/goleak"
)

func TestLeak(t *testing.T) {
  defer goleak.VerifyNone(t)

  gomega.RegisterFailHandler(ginkgo.Fail)
  ginkgo.RunSpecs(t, "Test")
}' > gokingoleak_test.go
go mod init gokingoleak
go mod tidy
go test
Running Suite: Test - /tmp
==========================
Random Seed: 1709289977

Will run 0 of 0 specs

Ran 0 of 0 Specs in 0.000 seconds
SUCCESS! -- 0 Passed | 0 Failed | 0 Pending | 0 Skipped
--- FAIL: TestLeak (0.44s)
    gokingoleak_test.go:16: found unexpected goroutines:
        [Goroutine 37 in state select, with github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2 on top of the stack:
        github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts.func2(0x0?)
        	/Users/alexandr.sudakov/go/pkg/mod/github.com/onsi/ginkgo/[email protected]/internal/interrupt_handler/interrupt_handler.go:131 +0x74
        created by github.com/onsi/ginkgo/v2/internal/interrupt_handler.(*InterruptHandler).registerForInterrupts in goroutine 35
        	/Users/alexandr.sudakov/go/pkg/mod/github.com/onsi/ginkgo/[email protected]/internal/interrupt_handler/interrupt_handler.go:128 +0x16c
        ]
FAIL
exit status 1
FAIL	gokingoleak	1.197s

It looks like the following diff can fix this problem:

diff --git a/core_dsl.go b/core_dsl.go
index 0e633f3..ed23330 100644
--- a/core_dsl.go
+++ b/core_dsl.go
@@ -297,7 +297,9 @@ func RunSpecs(t GinkgoTestingT, description string, args ...interface{}) bool {
 	suitePath, err = filepath.Abs(suitePath)
 	exitIfErr(err)
 
-	passed, hasFocusedTests := global.Suite.Run(description, suiteLabels, suitePath, global.Failer, reporter, writer, outputInterceptor, interrupt_handler.NewInterruptHandler(client), client, internal.RegisterForProgressSignal, suiteConfig)
+	interruptHandler := interrupt_handler.NewInterruptHandler(client)
+	defer interruptHandler.Stop()
+	passed, hasFocusedTests := global.Suite.Run(description, suiteLabels, suitePath, global.Failer, reporter, writer, outputInterceptor, interruptHandler, client, internal.RegisterForProgressSignal, suiteConfig)
 	outputInterceptor.Shutdown()
 
 	flagSet.ValidateDeprecations(deprecationTracker)

Though, I'm not sure if this is the best way to do it.

@onsi
Copy link
Owner

onsi commented Mar 1, 2024

yes - there are goroutines that outlive each individual spec and that outlive the entire test. the fix you propose would fix the leak that outlives the entire test. fwiw - you may want to use gleak: https://onsi.github.io/gomega/#codegleakcode-finding-leaked-goroutines - it’s tailored to work with Ginkgo and Gomega and would help you confirm that your tests don’t leak at a more granular level (i.e. no individual spec should leak - not just the entire suite). this is useful to debug when leaks occur. it also has the advantage of being aware of ginkgo’s goroutines and ignoring them.

@cyga
Copy link
Author

cyga commented Mar 1, 2024

@onsi thanks for a quick response and advice about gleak. I already saw gleak mentioned in one of open issues. I am going to try to adopt it as a workaround for automatic goroutine leak testing in my company's monorepo.

However, as far as I understood there is no need for the interrupt handler goroutine to outlive the whole suite. Thus, this is a good idea to change this behaviour. Can you confirm that?

@onsi
Copy link
Owner

onsi commented Mar 1, 2024

yes that’s also correct - if you want to submit a PR please do

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

No branches or pull requests

2 participants