Skip to content

Fix a flaky check and a TSan violation in StateFlowStressTest. #4483

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

chaoren
Copy link
Contributor

@chaoren chaoren commented Jul 18, 2025

There's no guarantee that any particular collector will ever encounter a value emitted by a particular emitter. It's entirely possible for the value to be overwritten by a different emitter before the collector gets a chance to collect it. It's very unlikely for a collector to miss the second half of all values emitted by a particular emitter, but it is still possible and this causes the test to be flaky.

@chaoren
Copy link
Contributor Author

chaoren commented Jul 18, 2025

I guess if you want to check that none of the collectors "hanged up" then you could check that c.max() is sufficiently high?

@dkhalanskyjb
Copy link
Collaborator

Yes, good idea. Could you please add that check?

@chaoren chaoren force-pushed the StateFlowStressTest branch from e0cf96d to c0f5474 Compare July 24, 2025 23:05
@chaoren chaoren changed the title Remove flaky check from StateFlowStressTest. Fix flaky check in StateFlowStressTest. Jul 24, 2025
@chaoren
Copy link
Contributor Author

chaoren commented Jul 24, 2025

Done. I noticed that the relative values between different emitters doesn't actually matter. They can each emit values at an arbitrary pace, so it doesn't make sense to use c.max(). I just changed it to make sure that the collector collected recent enough values from at least one emitter. I also added some changes to fix a TSan violation.

@chaoren chaoren force-pushed the StateFlowStressTest branch from c0f5474 to 997cdcf Compare July 24, 2025 23:13
@chaoren chaoren changed the title Fix flaky check in StateFlowStressTest. Fix a flaky check in StateFlowStressTest and a TSan violation. Jul 24, 2025
@chaoren chaoren force-pushed the StateFlowStressTest branch from 997cdcf to e50f612 Compare July 24, 2025 23:15
@chaoren chaoren changed the title Fix a flaky check in StateFlowStressTest and a TSan violation. Fix a flaky check and a TSan violation in StateFlowStressTest. Jul 24, 2025
There's no guarantee that any particular collector will ever encounter
a value emitted by a particular emitter. It's entirely possible for the
value to be overwritten by a different emitter before the collector gets
a chance to collect it. It's very unlikely for a collector to miss the
second half of all values emitted by a particular emitter, but it is
still possible and this causes the test to be flaky.

We can instead check if the collector has collected a recent enough
value from *any* emitter. This should be sufficient to verify that the
collector was still running near the end of the test.

Also fixed a race condition in the test when printing test progress.
The race condition is benign, but it causes TSan to fail for the test,
which could prevent it from finding other concurrency bugs.
@chaoren chaoren force-pushed the StateFlowStressTest branch from e50f612 to 3527031 Compare July 24, 2025 23:24
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