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

Make lock-related tests not flaky #211

Merged

Conversation

marcin-krystianc
Copy link
Contributor

@marcin-krystianc marcin-krystianc commented Mar 24, 2023

Fixes #210

After many experiments, I've finally found that flaky lock-related tests were caused by a low initial value of worker threads in the thread pool. It led to deadlock-like scenarios which resulted in expiring sessions due to missed renewal window.

@marcin-krystianc marcin-krystianc marked this pull request as ready for review March 30, 2023 15:36
@mfkl
Copy link
Contributor

mfkl commented Mar 31, 2023

Wow, interesting find... I see that you used GetMinThreads before but decided on using 32, 4 in the end. Why is that? Would this risk breaking silently when they change the runners?

Good job!

@marcin-krystianc
Copy link
Contributor Author

I see that you used GetMinThreads before but decided on using 32, 4 in the end. Why is that?

Well, GetMinThreads was only for debugging purposes, wanted to make sure that my theory is true.
When we override these values with SetMinThreads there is no need to call GetMinThreads anymore.
It seems that xUnit sets the number of worker threads equal to the number of CPU cores and GH runners have only 2 cores (or 3 cores for MacOS). Also, the test failures happened only for the net461 target, apparently .NET Core can quickly add new threads to the pool whereas .NET Framework is unable to do it quickly enough.

PS:
Also, worth mentioning this blog post which was really helpful: https://andrewlock.net/tracking-down-a-hanging-xunit-test-in-ci-building-a-custom-test-framework/.

mfkl
mfkl previously approved these changes Mar 31, 2023
@marcin-krystianc
Copy link
Contributor Author

FYI, I've checked how many threads are initially created by console applications and it turns out that number is also equal to the number of logical cores. So to avoid creating "too many" threads in the thread pool I've just pushed a change that bumps the number of worker threads only if the initial number is less than 4.

@marcin-krystianc marcin-krystianc merged commit 309ab12 into G-Research:master Mar 31, 2023
@marcin-krystianc marcin-krystianc deleted the marcink-20230324-locks branch March 31, 2023 10:30
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.

[Bug]: Consul.Test.SemaphoreTest.Semaphore_OneShot is flaky
2 participants