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

Use CDL to block threads to avoid flaky tests. #14116

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

aoli-al
Copy link
Contributor

@aoli-al aoli-al commented Jan 8, 2025

Description

This PR fixes a flaky test in testIntraMergeThreadPoolIsLimitedByMaxThreads. The test uses sleep to block thread execution which is not deterministic due to concurrency issues. This PR replaces the sleep statement with a CountDownLatch.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Hey! Getting rid of Thread.sleep! nice.

I ran the test many thousands of times off of main and here, with various amounts of thread switching due to running stress-ng in a different terminal (thus exercising my CPU threads and kicking the process off).

It never failed, but getting rid of sleep is justification enough IMO. I will merge and backport.

Maybe add an entry in CHANGES for the next 10.x release under the

Other
---------------------

tag?

@benwtrent benwtrent self-assigned this Jan 9, 2025
@aoli-al
Copy link
Contributor Author

aoli-al commented Jan 9, 2025

Thanks! I've updated the CHANGES.txt file.

BTW, we are building a concurrency testing framework (Fray) that allows developers to test concurrent programs deterministically under different thread interleavings. I tested this fix with Fray for 10 minus, and Fray did not find any bugs.

In addition to this bug, Fray has found several concurrency bugs, including #13593, #13571, #13552, and #13547. Feel free to try it and send us your feedback. We are trying to make it more developer-friendly these days.

@benwtrent
Copy link
Member

Very useful @aoli-al thank you!

@benwtrent benwtrent merged commit ee65e8f into apache:main Jan 9, 2025
5 checks passed
benwtrent pushed a commit that referenced this pull request Jan 9, 2025
* Use CDL to block threads to avoid flaky tests.

* Update CHANGES.txt
@dweiss
Copy link
Contributor

dweiss commented Jan 10, 2025

BTW, we are building a concurrency testing framework (Fray) that allows developers to test concurrent programs deterministically under different thread interleavings

That's very interesting. Is there any writeup (other than the code) that explains how you're doing this?

@aoli-al
Copy link
Contributor Author

aoli-al commented Jan 10, 2025

@dweiss Yes, we have a paper on Fray https://aoli.al/papers/fray-arxiv.pdf. I'm also working on blogs to discuss Fray in more detail. I'll let you know as soon as they are ready.

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.

3 participants