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 race in testutil Accumulator.Wait() #2598

Merged
merged 1 commit into from
Mar 30, 2017
Merged

fix race in testutil Accumulator.Wait() #2598

merged 1 commit into from
Mar 30, 2017

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Mar 29, 2017

Noticed this in a test result for #2587: https://circleci.com/gh/influxdata/telegraf/6432?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

What happens is that the accumulator has 2 metrics in it, Wait(3) is called and grabs a lock. Then another goroutine adds a metric, increments the atomic counter, and blocks waiting for the lock. The first goroutine, which is still running, gets to the point in the code where it checks the atomic counter, sees 3, and returns, releasing the lock. This frees up the second goroutine, but it doesn't get scheduled on the CPU yet. The caller of Wait(3) then tries to access the 3rd metric, but the second goroutine didn't get a chance to add it yet.

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)

@danielnelson
Copy link
Contributor

What about in ClearMetrics()?

@phemmer
Copy link
Contributor Author

phemmer commented Mar 29, 2017

Good catch. updated

@danielnelson
Copy link
Contributor

I wouldn't bother with the changelog since it only affects developers.

@phemmer
Copy link
Contributor Author

phemmer commented Mar 30, 2017

Removed (forgot to go back and add PR number anyway :-P )

@danielnelson danielnelson merged commit 03ee602 into influxdata:master Mar 30, 2017
@phemmer phemmer deleted the fix-test-accumulator-wait-race branch March 30, 2017 00:03
calerogers pushed a commit to calerogers/telegraf that referenced this pull request Apr 5, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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.

2 participants