Skip to content

Yield from the main thread a few times at shutdown #2660

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Yield from the main thread a few times at shutdown #2660

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
929e80c
Yield from the main thread a few times at shutdown
saethlin Nov 9, 2022
fe07f66
Update src/concurrency/thread.rs
saethlin Nov 20, 2022
eefbf1d
Turn check_terminated into should_terminate, which does not modify th…
saethlin Nov 20, 2022
624a593
Better organization and wording
saethlin Nov 20, 2022
b40c64c
Yield after TLS dtors, not before
saethlin Nov 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions src/concurrency/thread.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ pub struct ThreadId(u32);
/// The main thread. When it terminates, the whole application terminates.
const MAIN_THREAD: ThreadId = ThreadId(0);

/// When the main thread would exit, we will yield to any other thread that is ready to execute.
/// But we must only do that a finite number of times, or a background thread running `loop {}`
/// will hang the program.
const MAIN_THREAD_YIELDS_AT_SHUTDOWN: u64 = 1_000;

impl ThreadId {
pub fn to_u32(self) -> u32 {
self.0
Expand Down Expand Up @@ -131,16 +136,9 @@ pub struct Thread<'mir, 'tcx> {
}

impl<'mir, 'tcx> Thread<'mir, 'tcx> {
/// Check if the thread is done executing (no more stack frames). If yes,
/// change the state to terminated and return `true`.
fn check_terminated(&mut self) -> bool {
if self.state == ThreadState::Enabled {
if self.stack.is_empty() {
self.state = ThreadState::Terminated;
return true;
}
}
false
/// Check if the thread is done executing (no more stack frames).
fn should_terminate(&self) -> bool {
self.state == ThreadState::Enabled && self.stack.is_empty()
}

/// Get the name of the current thread, or `<unnamed>` if it was not set.
Expand Down Expand Up @@ -276,6 +274,9 @@ pub struct ThreadManager<'mir, 'tcx> {
yield_active_thread: bool,
/// Callbacks that are called once the specified time passes.
timeout_callbacks: FxHashMap<ThreadId, TimeoutCallbackInfo<'mir, 'tcx>>,
/// When the main thread is about to exit, we give other threads a few chances to finish up
/// whatever they are doing before we consider them leaked.
main_thread_yields_at_shutdown_remaining: u64,
}

impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> {
Expand All @@ -290,6 +291,7 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> {
thread_local_alloc_ids: Default::default(),
yield_active_thread: false,
timeout_callbacks: FxHashMap::default(),
main_thread_yields_at_shutdown_remaining: MAIN_THREAD_YIELDS_AT_SHUTDOWN,
}
}
}
Expand All @@ -302,6 +304,7 @@ impl VisitTags for ThreadManager<'_, '_> {
timeout_callbacks,
active_thread: _,
yield_active_thread: _,
main_thread_yields_at_shutdown_remaining: _,
sync,
} = self;

Expand Down Expand Up @@ -620,11 +623,25 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
/// long as we can and switch only when we have to (the active thread was
/// blocked, terminated, or has explicitly asked to be preempted).
fn schedule(&mut self, clock: &Clock) -> InterpResult<'tcx, SchedulingAction> {
// Check whether the thread has **just** terminated (`check_terminated`
// checks whether the thread has popped all its stack and if yes, sets
// the thread state to terminated).
if self.threads[self.active_thread].check_terminated() {
return Ok(SchedulingAction::ExecuteDtors);
// If we are the main thread, and our call stack is empty but our state is Enabled, we are
// about to terminate.
// But, if there are any other threads which can execute, yield to them instead of falling
// through to the termination state. This is to give them a chance to clean up and quit.
let active_thread = &self.threads[self.active_thread];
if self.active_thread == MAIN_THREAD
&& active_thread.should_terminate()
&& self.threads.iter().any(|t| t.state == ThreadState::Enabled && !t.stack.is_empty())
&& self.main_thread_yields_at_shutdown_remaining > 0
{
self.yield_active_thread = true;
self.main_thread_yields_at_shutdown_remaining -= 1;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd do this yielding after executing main thread TLS dtors (because it'd be UB to access them). Not sure how terrible that would be to implement though, so a FIXME is also fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... I would like to understand how to do this correctly. The naive implementation is just shifting around the big check this PR adds so that the original if should_terminate() { return Ok(ExecuteDtors) } comes first, but that seems to produce data races whenever this yielding behavior kicks in: We run the main thread's dtors, then schedule another thread on the next step, and that thread reports a data race on one of a few allocations in the Rust runtime.

It wouldn't surprise me if these data races are legitimate. After all, we do have this comment:

                // The main thread terminated; stop the program.
                // We do *not* run TLS dtors of remaining threads, which seems to match rustc behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Hard to say more without knowing the details of the race and how you implemented this. But we already currently will automatically yield to other threads while main thread Dtors are running, so we should already be seeing such races?

The best place to put this yielding behavior seems to be in the if self.threads[MAIN_THREAD].state == ThreadState::Terminated check, no? Instead of stopping, we keep going, but with some kind of bound.

Copy link
Member

Choose a reason for hiding this comment

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

Actually a bound on the steps is probably better than yielding... if automatic yielding is disabled, this might otherwise keep going forever even after the main thread stopped.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a commit which does the change I was trying to describe above. I've seen two data races reported, one in the test suite and the other when I run this file:

use std::thread;

fn main() {
    thread::scope(|s| {
        s.spawn(|| {});
    });
}

With ./miri many-seeds ./miri demo.rs

I agree that a bound on the extra steps sounds reasonable. Is the concern that this yielding behavior may take programs which currently exit under the non-yielding option and turn them into programs that never exit?

Copy link
Member

Choose a reason for hiding this comment

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

Is the concern that this yielding behavior may take programs which currently exit under the non-yielding option and turn them into programs that never exit?

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The TLS shims re-enable terminated threads. Is that a good thing? Do they have to be Enabled in order to execute user code?

Copy link
Member

@RalfJung RalfJung Nov 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, threads must definitely be enabled when running TLS dtors.

I wish they wouldn't be disabled and then re-enabled... this entire scheduler and TLS stuff needs a rework at some point, but I haven't yet found a nice way to factor this. I still think a generator would be good... basically yielding a function to run, which is then executed until the stack is empty again, at which point the generator is consulted to figure out whether there is another function to run or whether this thread is done.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sadly using generators for this will not work.

} else {
// Check whether the thread ought to terminate, and if so start executing TLS
// destructors.
if self.threads[self.active_thread].should_terminate() {
self.threads[self.active_thread].state = ThreadState::Terminated;
return Ok(SchedulingAction::ExecuteDtors);
}
}
// If we get here again and the thread is *still* terminated, there are no more dtors to run.
if self.threads[MAIN_THREAD].state == ThreadState::Terminated {
Expand Down Expand Up @@ -671,6 +688,15 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
}
}
self.yield_active_thread = false;

// The main thread is special, it is allowed to yield when it is shutting down and has
// nothing to execute. So if another thread yields back to the main thread when it is in
// this partly-shut-down state, we make another pass through the scheduler to either yield
// back off them main thread, or enter its shutdown sequence.
if self.active_thread == MAIN_THREAD && self.threads[self.active_thread].stack.is_empty() {
return self.schedule(clock);
}

if self.threads[self.active_thread].state == ThreadState::Enabled {
return Ok(SchedulingAction::ExecuteStep);
}
Expand Down