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: clean-up the rate sequence goroutines to avoid leaking #142

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

d1823
Copy link
Contributor

@d1823 d1823 commented Aug 23, 2023

As stated on Ticker.Stop:

Stop does not close the channel, to prevent a concurrent goroutine
reading from the channel from seeing an erroneous "tick".

This sole fact prevents the goroutine from exiting, and in result, causes a leak that is especially problematic when the project is used through an API and not a CLI.

This particular bug is directly affecting the Telegraf's Internet Speed Input Plugin, so this fix is only resolving the part relevant to the plugin.

There are two more occurrences of this pattern:

func (dm *DataManager) CallbackDownloadRate(callback func(downRate float64)) *time.Ticker {
func (dm *DataManager) CallbackUploadRate(callback func(upRate float64)) *time.Ticker {

Unfortunately, fixing them would probably require an API change, so I'm purposefully leaving that to the maintainers of this project.

As stated on Ticker.Stop:

    Stop does not close the channel, to prevent a concurrent goroutine
    reading from the channel from seeing an erroneous "tick".

This sole fact prevents the goroutine from exiting, and in result,
causes a leak that is especially problematic when the project is used as
through an API and not a CLI.
@r3inbowari
Copy link
Collaborator

@d1823 Thank you very much for your careful debugging! I made a release for telegraf's quickly fixing, and another occurrences(CLI) can be fix in the next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants