fix: goroutine leak #5364

0xff-dev · 2023-10-08T07:45:51Z

fix: goroutine leak

k8s-ci-robot · 2023-10-08T07:45:59Z

Welcome @0xff-dev!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-10-08T07:45:59Z

Hi @0xff-dev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

charles-chenzz · 2023-10-09T02:11:57Z

/ok-to-test

koba1t · 2023-10-14T16:20:18Z

@0xff-dev

Thanks for your contribution!

Please add any tests that check this bug fixed.
And it looks like any tests failed on CI, Could you check and fix it?

0xff-dev · 2023-10-15T06:18:11Z

@0xff-dev

Thanks for your contribution!

Please add any tests that check this bug fixed. And it looks like any tests failed on CI, Could you check and fix it?

Hi @koba1t , The code has been updated. During my local testing, I encountered a failure in the last test when utilizing an unbuffered channel.

0xff-dev · 2023-10-15T14:52:34Z

@koba1t Based on the execution results of the two change actions, it appears that the Test Linux test may be a flaky test.
https://github.com/kubernetes-sigs/kustomize/actions/runs/6522412115
https://github.com/kubernetes-sigs/kustomize/actions/runs/6446341552

charles-chenzz · 2023-10-16T01:32:13Z

api/internal/utils/timedcall_test.go

+	_ = TimedCall("expect no goroutine leaks", time.Second, func() error {
+		time.Sleep(2 * time.Second)
+		return fmt.Errorf("not done")
+	})


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

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

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

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

stormqueen1990

Just a few small suggestions

stormqueen1990 · 2023-11-04T21:58:22Z

api/internal/utils/timedcall_test.go

+	if beforeGroutine != afterGoroutine {
+		t.Fatal("goroutine leak")
+	}


Small suggestion:

Suggested change

if beforeGroutine != afterGoroutine {

t.Fatal("goroutine leak")

}

require.NotEqual(t, beforeGoroutine, afterGoroutine, "goroutine leak")

Hi @stormqueen1990 , I think it should be require.Equal here. Because we modified the unbuffered channel to be buffered, it should be equal when the test is done here.

Yes! I forgot to invert the test when I typed, sorry about that 😅

stormqueen1990 · 2023-11-04T21:59:08Z

api/internal/utils/timedcall_test.go

@@ -62,3 +63,15 @@ func TestTimedCallSlowWithError(t *testing.T) {
 		t.Fail()
 	}
 }
+
+func TestTimedCallGoroutineLeak(t *testing.T) {
+	beforeGroutine := runtime.NumGoroutine()


Small typo in variable name:

Suggested change

beforeGroutine := runtime.NumGoroutine()

beforeGoroutine := runtime.NumGoroutine()

charles-chenzz

just one minor comment

charles-chenzz · 2023-11-06T00:58:32Z

api/internal/utils/timedcall_test.go

+
+func TestTimedCallGoroutineLeak(t *testing.T) {
+	beforeGroutine := runtime.NumGoroutine()
+	_ = TimedCall("expect no goroutine leaks", time.Second, func() error {


why we don't catch the error here? are you ignore for some reasons?

I think the function has no special complex logic. So ignore the error.
I just looked at the other test examples and none of them ignored the error, so i need to change it, thanks for the review.

0xff-dev · 2023-11-06T02:41:10Z

@charles-chenzz @stormqueen1990 @koba1t I want to use go.uber.org/goleak to check for goroutine leaks, there may be some problems with the method I wrote before, do you think it's ok?

charles-chenzz · 2023-11-06T03:47:37Z

@charles-chenzz @stormqueen1990 @koba1t I want to use go.uber.org/goleak to check for goroutine leaks, there may be some problems with the method I wrote before, do you think it's ok?

you can push up an quick example to demonstrate it

natasha41575 · 2023-11-06T19:01:05Z

api/go.mod

@@ -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


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.

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.

charles-chenzz

/lgtm
/assign @natasha41575 @koba1t

k8s-ci-robot · 2023-11-10T16:01:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xff-dev, natasha41575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 8, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 8, 2023

k8s-ci-robot requested review from koba1t and yuwenma October 8, 2023 07:46

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2023

0xff-dev force-pushed the master branch from e96e992 to f8eed9a Compare October 15, 2023 06:15

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 15, 2023

0xff-dev force-pushed the master branch from f8eed9a to 52e167e Compare October 15, 2023 14:45

charles-chenzz reviewed Oct 16, 2023

View reviewed changes

0xff-dev force-pushed the master branch from 52e167e to 7b3ce53 Compare October 16, 2023 05:27

stormqueen1990 reviewed Nov 4, 2023

View reviewed changes

charles-chenzz reviewed Nov 6, 2023

View reviewed changes

fix: goroutine leak

f7bc0ac

0xff-dev force-pushed the master branch from 7b3ce53 to f7bc0ac Compare November 6, 2023 05:12

natasha41575 reviewed Nov 6, 2023

View reviewed changes

charles-chenzz reviewed Nov 9, 2023

View reviewed changes

k8s-ci-robot assigned koba1t, natasha41575 and charles-chenzz Nov 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2023

natasha41575 approved these changes Nov 10, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2023

k8s-ci-robot merged commit 863ca93 into kubernetes-sigs:master Nov 10, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: goroutine leak #5364

fix: goroutine leak #5364

0xff-dev commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

charles-chenzz commented Oct 9, 2023

koba1t commented Oct 14, 2023

0xff-dev commented Oct 15, 2023

0xff-dev commented Oct 15, 2023

charles-chenzz Oct 16, 2023

0xff-dev Oct 16, 2023 •

edited

charles-chenzz Oct 16, 2023

0xff-dev Oct 16, 2023

stormqueen1990 left a comment

stormqueen1990 Nov 4, 2023

0xff-dev Nov 5, 2023

stormqueen1990 Nov 5, 2023

stormqueen1990 Nov 4, 2023

charles-chenzz left a comment

charles-chenzz Nov 6, 2023

0xff-dev Nov 6, 2023

0xff-dev commented Nov 6, 2023 •

edited

charles-chenzz commented Nov 6, 2023

natasha41575 Nov 6, 2023

charles-chenzz Nov 7, 2023

charles-chenzz left a comment

k8s-ci-robot commented Nov 10, 2023

	beforeGroutine := runtime.NumGoroutine()
	beforeGoroutine := runtime.NumGoroutine()

fix: goroutine leak #5364

fix: goroutine leak #5364

Conversation

0xff-dev commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

k8s-ci-robot commented Oct 8, 2023

charles-chenzz commented Oct 9, 2023

koba1t commented Oct 14, 2023

0xff-dev commented Oct 15, 2023

0xff-dev commented Oct 15, 2023

Choose a reason for hiding this comment

0xff-dev Oct 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormqueen1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-chenzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xff-dev commented Nov 6, 2023 • edited

charles-chenzz commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-chenzz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2023

0xff-dev Oct 16, 2023 •

edited

0xff-dev commented Nov 6, 2023 •

edited