Prevent panics in worker threads from causing deadlocks #7218
+84
−125
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
Occasionally, we'll run into panics during compilation which prior to this diff would cause the compiler to hang indefinitely after printing the initial panic message. Note that this simplified/renamed worker 'wakeup' system seems to leave around a bit of a code smell - that this perhaps indicates we should rework/refactor in order to avoid the necessity of having separate wakeup and message stealing systems... but one problem at a time!
joshuawarner32
force-pushed
the
clean-shutdown
branch
from
5b368c6
to
d6b7ee2
Compare
Comment on lines
+1990
to
+1997
| // Careful! It's important that worker listeners aren't allocated in the arena, | ||
| // since they need to be correctly dropped if we have a panic in this thread::scope code. | ||
| // Making sure they're owned means they'll be dropped correctly on either normal exit | ||
| // of this thread::scope block or on panicking. When they're dropped, the worker threads | ||
| // will correctly exit their message processing loops. | ||
| // If these were allocated in the arena, we might panic without shutting down the worker threads, | ||
| // causing the thread::scope block to hang while it waits for the worker threads to exit. | ||
| let mut worker_wakers = Vec::with_capacity(num_workers); |
There was a problem hiding this comment.
Really helpful explanation 😄
lukewilliamboswell
approved these changes
Nov 14, 2024
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.
Occasionally, we'll run into panics during compilation which prior to this diff
would cause the compiler to hang indefinitely after printing the initial panic message.
Note that this simplified/renamed worker 'wakeup' system seems to leave around a bit of a code smell - that this perhaps indicates we should rework/refactor in order to avoid the necessity of having separate wakeup and message stealing systems... but one problem at a time!