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.

Sign up for GitHub

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

Jump to bottom

fix: goroutine leak #5364

Merged
merged 1 commit into from Nov 10, 2023
+22 −1
Merged

fix: goroutine leak #5364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions api/go.mod
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ require (
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
github.com/imdario/mergo v0.3.13
github.com/stretchr/testify v1.8.1
go.uber.org/goleak v1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a note here for other reviewers: I double checked if this dependency is OK by checking the allowlist here . MIT licenses are allowed, which looks like this repo is covered under, so this should be OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also check k/k, most of the deps on it are MIT and apache2. so I think goleak from uber which MIT license as well will be OK.

gopkg.in/evanphx/json-patch.v5 v5.6.0
gopkg.in/yaml.v2 v2.4.0
k8s.io/kube-openapi v0.0.0-20230601164746-7562a1006961
Expand Down
2 changes: 2 additions & 0 deletions api/go.sum
View file
Open in desktop
Expand Up @@ -61,6 +61,8 @@ github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ=
github.com/xlab/treeprint v1.2.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0=
go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5 h1:+FNtrFTmVw0YZGpBGX56XDee331t6JAXeK2bcyhLOOc=
go.starlark.net v0.0.0-20200306205701-8dd3e2ee1dd5/go.mod h1:nmDLcffg48OtT/PSW0Hg7FvpRQsQh5OSqIylirxKC7o=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
golang.org/x/sys v0.0.0-20191002063906-3421d5a6bb1c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Expand Down
2 changes: 1 addition & 1 deletion api/internal/utils/timedcall.go
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ import (
// TimedCall runs fn, failing if it doesn't complete in the given duration.
// The description is used in the timeout error message.
func TimedCall(description string, d time.Duration, fn func() error) error {
done := make(chan error)
done := make(chan error, 1)
timer := time.NewTimer(d)
defer timer.Stop()
go func() { done <- fn() }()
Expand Down
18 changes: 18 additions & 0 deletions api/internal/utils/timedcall_test.go
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"go.uber.org/goleak"
. "sigs.k8s.io/kustomize/api/internal/utils"
)

Expand Down Expand Up @@ -62,3 +63,20 @@ func TestTimedCallSlowWithError(t *testing.T) {
t.Fail()
}
}

func TestTimedCallGoroutineLeak(t *testing.T) {
defer goleak.VerifyNone(t)
err := TimedCall("function done, no goroutine leaks", timeToWait, func() error {
time.Sleep(tooSlow)
return fmt.Errorf("function done")
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you be more specify on what has not done? it would help a lot. Thanks

Copy link
Contributor Author

@0xff-dev 0xff-dev Oct 16, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the misrepresentation here.
not done I would say would cause the goroutine to fail to exit and not finish.
Would it be better to change it to return fmt.Error("function done")? Or just return nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I‘m not buying just return nil, I think something like goroutine to fail to exit and not finish or function done is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, the code has been changed to return fmt.Errorf("function done")

if assert.Error(t, err) {
assert.EqualError(t, err, errMsg("function done, no goroutine leaks"))
} else {
t.Fail()
}

// The code introduces a 2-second sleep to allow the goroutine to complete its execution.
// Subsequently, it verifies if the goroutine created by the function exits as expected.
time.Sleep(tooSlow)
}