- ensure that thread locals are destructed when a spawned thread panics

- ensure that loom doesn't double panic on deadlock
- add a test to verify that thread locals are properly destructed on spawned threads

Author: Pointerbender

src/rt/execution.rs

@@ -207,13 +207,13 @@ impl Execution {
 
         self.threads.set_active(next);
 
-        // There is no active thread. Unless all threads have terminated, the
-        // test has deadlocked.
+        // There is no active thread. Unless all threads have terminated
+        // or the current thread is panicking, the test has deadlocked.
         if !self.threads.is_active() {
             let terminal = self.threads.iter().all(|(_, th)| th.is_terminated());
 
             assert!(
-                terminal,
+                terminal || std::thread::panicking(),
                 "deadlock; threads = {:?}",
                 self.threads
                     .iter()

src/rt/mod.rs

@@ -73,8 +73,27 @@ where
     trace!(thread = ?id, "spawn");
 
     Scheduler::spawn(Box::new(move || {
+        /// the given closure `f` may panic when executed.
+        /// when this happens, we still want to ensure that
+        /// thread locals are destructed. therefore, we set
+        /// up a guard that is dropped as part of the unwind
+        /// logic when `f` panics.
+        struct PanicGuard;
+        impl Drop for PanicGuard {
+            fn drop(&mut self) {
+                thread_done(false);
+            }
+        }
+
+        // set up the panic guard
+        let panic_guard = PanicGuard;
+
+        // execute the closure, note that `f()` may panic!
         f();
-        thread_done(false);
+
+        // if `f()` didn't panic, then we terminate the
+        // spawned thread by dropping the guard ourselves.
+        drop(panic_guard);
     }));
 
     id
@@ -172,6 +191,21 @@ where
 }
 
 pub fn thread_done(is_main_thread: bool) {
+    let is_active = execution(|execution| execution.threads.is_active());
+    if !is_active {
+        // if the thread is not active and the current thread is panicking,
+        // then this means that loom has detected a problem (e.g. a deadlock).
+        // we don't want to throw a double panic here, because this would cause
+        // the entire test to abort and this hides the error from the end user.
+        // instead we ensure that the current thread is panicking already,
+        // or we cause a panic if it's not yet panicking (which it otherwise
+        // would anyway, on the call to `execution.threads.active_id()` below).
+        let panicking = std::thread::panicking();
+        trace!(?panicking, "thread_done: no active thread");
+        assert!(panicking);
+        return;
+    }
+
     let locals = execution(|execution| {
         let thread = execution.threads.active_id();
 

tests/deadlock.rs

@@ -6,7 +6,7 @@ use loom::thread;
 use std::rc::Rc;
 
 #[test]
-#[should_panic]
+#[should_panic(expected = "deadlock; threads =")]
 fn two_mutexes_deadlock() {
     loom::model(|| {
         let a = Rc::new(Mutex::new(1));

tests/thread_local.rs

@@ -162,15 +162,77 @@ fn lazy_static_panic() {
 
     impl Drop for Bar {
         fn drop(&mut self) {
+            assert!(BAR.try_with(|_| ()).is_err());
             let _ = &*ID;
         }
     }
 
     loom::model(|| {
         // initialize the TLS destructor:
         BAR.with(|_| ());
-        println!("about to panic");
         // intentionally panic:
         panic!("loom should not panic inside another panic");
     });
 }
+
+/// Test that thread locals are properly destructed
+/// when a spawned thread panics, without causing
+/// a double panic.
+#[test]
+#[should_panic(expected = "loom should not panic inside another panic")]
+fn access_on_drop_during_panic_in_spawned_thread() {
+    use loom::{cell::Cell, thread};
+    use std::{
+        panic::catch_unwind,
+        sync::{Mutex, MutexGuard, PoisonError},
+    };
+
+    struct DropCounter {
+        instantiated: usize,
+        dropped: usize,
+    }
+
+    static DROPPED: Mutex<DropCounter> = Mutex::new(DropCounter {
+        instantiated: 0,
+        dropped: 0,
+    });
+
+    loom::thread_local! {
+        static BAR: Cell<Bar> = Cell::new(Bar({
+            let mut guard = DROPPED.lock().unwrap();
+            guard.instantiated += 1;
+            guard
+        }));
+    }
+
+    struct Bar(MutexGuard<'static, DropCounter>);
+    impl Drop for Bar {
+        fn drop(&mut self) {
+            assert!(BAR.try_with(|_| ()).is_err());
+            self.0.dropped += 1;
+        }
+    }
+
+    let result = catch_unwind(|| {
+        loom::model(|| {
+            thread::spawn(|| {
+                // initialize the TLS destructor and panic:
+                BAR.with(|_| {
+                    BAR.with(|_| {
+                        panic!();
+                    });
+                });
+            })
+            .join()
+            .unwrap();
+        });
+    });
+
+    let guard = DROPPED.lock().unwrap_or_else(PoisonError::into_inner);
+    assert_eq!(guard.instantiated, 1);
+    assert_eq!(guard.dropped, 1);
+
+    // propagate the panic from the spawned thread
+    // to the main thread.
+    result.expect("loom should not panic inside another panic");
+}