From 929e80ce7d1f55a19a2c96f007c911daaa78da4a Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Tue, 8 Nov 2022 23:47:10 -0500 Subject: [PATCH 1/5] Yield from the main thread a few times at shutdown --- src/concurrency/thread.rs | 44 ++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index 8fbee9a352..d1fe3231da 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -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 @@ -276,6 +281,9 @@ pub struct ThreadManager<'mir, 'tcx> { yield_active_thread: bool, /// Callbacks that are called once the specified time passes. timeout_callbacks: FxHashMap>, + /// 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> { @@ -290,6 +298,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, } } } @@ -302,6 +311,7 @@ impl VisitTags for ThreadManager<'_, '_> { timeout_callbacks, active_thread: _, yield_active_thread: _, + main_thread_yields_at_shutdown_remaining: _, sync, } = self; @@ -620,11 +630,26 @@ 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. + let active_thread = &self.threads[self.active_thread]; + if self.active_thread == MAIN_THREAD + && active_thread.stack.is_empty() + && active_thread.state == ThreadState::Enabled + && 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; + } else { + // 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 get here again and the thread is *still* terminated, there are no more dtors to run. if self.threads[MAIN_THREAD].state == ThreadState::Terminated { @@ -671,6 +696,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); } From fe07f66454a7b1d30df17884ec8462311d612ec5 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 19 Nov 2022 19:40:29 -0500 Subject: [PATCH 2/5] Update src/concurrency/thread.rs Co-authored-by: Ralf Jung --- src/concurrency/thread.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index d1fe3231da..afac0d63ff 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -633,7 +633,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // 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. + // 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.stack.is_empty() From eefbf1d8120e93e9cc63dea83b04332365c3bbae Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 19 Nov 2022 20:02:55 -0500 Subject: [PATCH 3/5] Turn check_terminated into should_terminate, which does not modify the Thread --- src/concurrency/thread.rs | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index afac0d63ff..b59fcc8d25 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -136,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 `` if it was not set. @@ -636,18 +629,17 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // 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.stack.is_empty() - && active_thread.state == ThreadState::Enabled + && 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; } else { - // 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() { + // 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); } } From 624a5930d8eaeeedcf19318e9f026df25f1db00b Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 20 Nov 2022 18:30:04 -0500 Subject: [PATCH 4/5] Better organization and wording --- src/concurrency/thread.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index b59fcc8d25..e305d43c1a 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -136,7 +136,7 @@ pub struct Thread<'mir, 'tcx> { } impl<'mir, 'tcx> Thread<'mir, 'tcx> { - /// Check if the thread is done executing (no more stack frames). + /// Check if the thread is just done executing (still enabled, but no more stack frames). fn should_terminate(&self) -> bool { self.state == ThreadState::Enabled && self.stack.is_empty() } @@ -689,15 +689,16 @@ 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 { + // 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); + } return Ok(SchedulingAction::ExecuteStep); } // We have not found a thread to execute. From b40c64c40049f3d914e7a3a507fe2533af4c08b2 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 20 Nov 2022 18:44:58 -0500 Subject: [PATCH 5/5] Yield after TLS dtors, not before --- src/concurrency/thread.rs | 47 +++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/concurrency/thread.rs b/src/concurrency/thread.rs index e305d43c1a..19aaa42a48 100644 --- a/src/concurrency/thread.rs +++ b/src/concurrency/thread.rs @@ -623,31 +623,34 @@ 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> { - // 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; - } 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); - } + // 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 { - // The main thread terminated; stop the program. - // We do *not* run TLS dtors of remaining threads, which seems to match rustc behavior. - return Ok(SchedulingAction::Stop); + // If we are the main thread and we are Terminated, we ought to stop. + // 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.state == ThreadState::Terminated + && 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; + } else { + // The main thread terminated; stop the program. + // We do *not* run TLS dtors of remaining threads, which seems to match rustc behavior. + return Ok(SchedulingAction::Stop); + } } // This thread and the program can keep going. if self.threads[self.active_thread].state == ThreadState::Enabled