htsthread_wait() has no regression test - #760
Merged
Merged
Conversation
htsselftest.c and tests/Makefile.am were held by #718 while #747 was fixed, so the thread-counting fix went in without a test. -#test=threadwait covers it from both sides: a wait placed right after a spawn joins that thread, and wait_n(n) leaves n running rather than draining them. One spawn per round is what makes it bite. A batch gives the earlier threads time to raise the counter themselves, which is why an eight-thread version passed on the unfixed engine; one thread per round failed 10 runs out of 10. The changes-race self-test can now drop the counter it kept because htsthread_wait() could not be trusted to join. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <xroche@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#747 landed without a test, because #718 held
htsselftest.candtests/Makefile.amat the time. This is that test.-#test=threadwaitchecks the contract from both ends: ahtsthread_wait()placed right after a spawn joins that thread, andhtsthread_wait_n(n)leavesnrunning instead of draining them. The gated threads used for the second half give up after ten seconds, so a wait that wrongly drains them fails the assertion rather than hanging the suite.The first version of it was useless and that is worth recording. Spawning eight threads per round, it passed on the unfixed engine every single time: the earlier threads in the batch raise the counter while the rest are still being created, so the wait never sees the zero it needs to return early. One spawn per round opens that window about half the time, and sixteen rounds make it a certainty. Reverted to its pre-#752 state,
src/htsthread.cnow fails the test 10 runs out of 10, and the current tree passes 10 out of 10.st_changes_racealso drops the counter it kept for joining its own notifier threads. That workaround is what turned up the bug in the first place.