fix: give task_done_cv its own mutex to prevent race conditions.
#71
Conversation
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
…other way (workers -> wait_for_tasks(), i.e. from workers to management threads, instead of `task_available_cv` + `task_mutex`, which are the pair communicating from anagement down to the workers collective ;-) ). As these two directions should be independent, so must be their mutexes. This prevents all kinds of possible weird lockups and miscommunication. --- From: SHA-1: 12253b6 * fixes: - push_task(): document which (member) variables are under which mutex' overwatch and makee sure the code matches this. + Case in point: [submitted in other PR] + Second case in point: `task_done_cv` has an opposing purpose and MUST be wrapped by its own mutex to prevent deadlock between wait_for_tasks() and any worker threads. Introducing `task_done_mutex` for that.
|
Thanks for opening this pull request! However, I'm not sure I understand why a second mutex is needed. Can you provide a minimal working example demonstrating a race condition? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe the changes
fix: give
task_done_cvits own mutex: it's for sending signals the other way (workers -> wait_for_tasks(), i.e. from workers to management threads, instead oftask_available_cv+task_mutex, which are the pair communicating from management down to the workers collective ;-) ). As these two directions should be independent, so must be their mutexes. This prevents all kinds of possible weird lockups and miscommunication.From: SHA-1: 12253b6
task_done_cvhas an opposing purpose and MUST be wrapped by its own mutex to prevent deadlock between wait_for_tasks() and any worker threads. Introducingtask_done_mutexfor that.Testing
This was tested as part of a larger work (other PRs are forthcoming shortly) after hunting down shutdown issues (application lockups, etc.) in a large application.
Tested this code via your provided test code rig; see my own fork and the referenced commits which point into there.
Tested on AMD Ryzen 3700X, 128GB RAM, latest Win10/64, latest MSVC2019 dev environment. Using in-house project files which use a (in-house) standardized set of optimizations.
Additional information
TBD
The patches are hopefully largely self-explanatory. Where deemed useful, the original commit messages from the dev fork have been referenced and included.