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 condition in WindowedThroughput sampler #73

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

kentquirk
Copy link
Contributor

Which problem is this PR solving?

  • A "flaky" test in Refinery would fail occasionally with a race condition in WindowedSampler. This fixes it.

Short description of the changes

  • Use a local temporary variable instead of modifying the count on the fly, then write it during the existing lock periods.

Copy link
Contributor

@fchikwekwe fchikwekwe left a comment

Choose a reason for hiding this comment

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

Oh, I see. Was this an issue with adjusting the struct values in place?

@fchikwekwe fchikwekwe merged commit 7a8d666 into main Jan 11, 2024
4 checks passed
@fchikwekwe fchikwekwe deleted the kent.fix_windowed_race branch January 11, 2024 23:59
@kentquirk
Copy link
Contributor Author

Basically, yes. It was changing t.numKeys without a lock around it. I could have locked it there, but instead I used a temp and modified t.numKeys inside the already-locked regions so that we didn't have to acquire another lock.

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