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

fix goroutine leaks, when using 'for range ticker.Context()' in gorou… #174

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

watjt
Copy link
Contributor

@watjt watjt commented Jul 29, 2021

"ticker.Context()" used in many places in the rancher, like this "for range ticker.Context() {do something}", the channel will not be closed for "ticker.Stop()", "for range ticker.Context() {do something}" outside the goroutine will never exit, cause goroutine leaks.

@dramich
Copy link
Contributor

dramich commented Aug 3, 2021

@watjt Thanks for the pr and bringing this up. I think this only solves part of the problem as the ticker channel will still not get closed and will still leak even with this fix. https://cs.opensource.google/go/go/+/refs/tags/go1.16.6:src/time/tick.go;l=44

@watjt
Copy link
Contributor Author

watjt commented Aug 4, 2021

@watjt Thanks for the pr and bringing this up. I think this only solves part of the problem as the ticker channel will still not get closed and will still leak even with this fix. https://cs.opensource.google/go/go/+/refs/tags/go1.16.6:src/time/tick.go;l=44

Yes, the ticker channel will still not get closed when goroutine where ticker is located still not exit. if we free *Ticker (Exit the goroutine where ticker is located), ticker channel will be gc. goroutine that uses' for range ticker.Context () 'will also exit. the main reason is that the goroutine here has leaked.

@ibuildthecloud
Copy link
Contributor

The ticker should get GC'd with this fix so the open channel won't matter.

LGTM

@dramich
Copy link
Contributor

dramich commented Aug 4, 2021

Sounds good, thanks for the fix @watjt!

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 this pull request may close these issues.

None yet

3 participants