Robustify multi-threading primitives #72249
Merged
Conversation
This file contains 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
RandomShaper
commented
Jan 28, 2023
| @@ -113,7 +113,9 @@ Thread::Thread() { | |||
| Thread::~Thread() { | |||
| if (id != _thread_id_hash(std::thread::id())) { | |||
| #ifdef DEBUG_ENABLED | |||
| WARN_PRINT("A Thread object has been destroyed without wait_to_finish() having been called on it. Please do so to ensure correct cleanup of the thread."); | |||
| WARN_PRINT( | |||
There was a problem hiding this comment.
I've just reworded this one to be consistent with the warning newly added to Semaphore.
|
Given 3.x is also affected (and has a fix ready), this should be safe to merge for 4.0.x when reduz has time to review it. |
RandomShaper
force-pushed
the
robust_sync
branch
from
03a9ab8 to
fb46f44
Compare
|
Pushed just fixing a couple of mistakes in the docs. |
RandomShaper
added
cherrypick:4.0
enhancement
platform:windows
and removed
platform:windows
labels
RandomShaper
force-pushed
the
robust_sync
branch
from
fb46f44 to
a6f883d
Compare
|
Rebased on top of 4.1. |
RandomShaper
force-pushed
the
robust_sync
branch
from
a6f883d to
6b5c38e
Compare
reduz
reviewed
May 15, 2023
reduz
approved these changes
May 15, 2023
|
Thanks! |
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.
This PR does the following:
Threadobjects were leaked to the OS if not joined (i.e., in those cases there would be no possibility for the warning about callingwait_to_finish()to be printed; therefore, pretty much hiding the issue to the user).Semaphore(long explanation in the code comments; TL;DR is Don't exit with semaphores being awaited) and avoids the undefined behavior. I have tried to add similar protection toMutex, but there are no reasonable ways to detect misuse. In any case, now we can finally get the warning for non-joined threads, most (all?) cases of misuse would cause some kind of admonishment.Fixes #71216.
UPDATE: Back-ported to 3.x in #72251.