- clarify documentation of mock lazy_static implementation

- ensure that global statics are dropped after thread locals
- ensure that `loom` does not panic inside a panic
- add two regression tests for #152

Author: Pointerbender

src/lazy_static.rs

@@ -1,4 +1,13 @@
 //! Mock implementation of the `lazy_static` crate.
+//!
+//! Note: unlike the [semantics] of the `lazy_static` crate, this
+//! mock implementation *will* drop its value when the program finishes.
+//! This is due to an implementation detail in `loom`: it will create
+//! many instances of the same program and this would otherwise lead
+//! to unbounded memory leaks due to instantiating the lazy static
+//! many times over.
+//!
+//! [semantics]: https://docs.rs/lazy_static/latest/lazy_static/#semantics
 
 use crate::rt;
 pub use crate::rt::thread::AccessError;
@@ -12,6 +21,15 @@ use std::fmt;
 use std::marker::PhantomData;
 
 /// Mock implementation of `lazy_static::Lazy`.
+///
+/// Note: unlike the [semantics] of the `lazy_static` crate, this
+/// mock implementation *will* drop its value when the program finishes.
+/// This is due to an implementation detail in `loom`: it will create
+/// many instances of the same program and this would otherwise lead
+/// to unbounded memory leaks due to instantiating the lazy static
+/// many times over.
+///
+/// [semantics]: https://docs.rs/lazy_static/latest/lazy_static/#semantics
 pub struct Lazy<T> {
     // Sadly, these fields have to be public, since function pointers in const
     // fns are unstable. When fn pointer arguments to const fns stabilize, these

src/model.rs

@@ -182,14 +182,37 @@ impl Builder {
             let f = f.clone();
 
             scheduler.run(&mut execution, move || {
-                f();
+                /// the given closure `f` may panic when executed.
+                /// when this happens, we still want to ensure
+                /// that thread locals are destructed before the
+                /// global statics are dropped. therefore, we set
+                /// up a guard that is dropped as part of the unwind
+                /// logic when `f` panics. this guard in turn ensures,
+                /// through the implementation of `rt::thread_done`,
+                /// that thread local fields are dropped before the
+                /// global statics are dropped. without this guard,
+                /// a `Drop` implementation on a type that is stored
+                /// in a thread local field could access the lazy static,
+                /// and this then would in turn panic from the method
+                /// [`Lazy::get`](crate::lazy_static::Lazy), which
+                /// causes a panic inside a panic, which in turn causes
+                /// Rust to abort, triggering a segfault in `loom`.
+                struct PanicGuard;
+                impl Drop for PanicGuard {
+                    fn drop(&mut self) {
+                        rt::thread_done(true);
+                    }
+                }
 
-                let lazy_statics = rt::execution(|execution| execution.lazy_statics.drop());
+                // set up the panic guard
+                let panic_guard = PanicGuard;
 
-                // drop outside of execution
-                drop(lazy_statics);
+                // execute the closure, note that `f()` may panic!
+                f();
 
-                rt::thread_done();
+                // if `f()` didn't panic, then we terminate the
+                // main thread by dropping the guard ourselves.
+                drop(panic_guard);
             });
 
             execution.check_for_leaks();

src/rt/mod.rs

@@ -74,7 +74,7 @@ where
 
     Scheduler::spawn(Box::new(move || {
         f();
-        thread_done();
+        thread_done(false);
     }));
 
     id
@@ -171,7 +171,7 @@ where
     Scheduler::with_execution(f)
 }
 
-pub fn thread_done() {
+pub fn thread_done(is_main_thread: bool) {
     let locals = execution(|execution| {
         let thread = execution.threads.active_id();
 
@@ -180,9 +180,28 @@ pub fn thread_done() {
         execution.threads.active_mut().drop_locals()
     });
 
-    // Drop outside of the execution context
+    // Drop locals outside of the execution context
     drop(locals);
 
+    if is_main_thread {
+        // Note that the statics must be dropped AFTER
+        // dropping the thread local fields. The `Drop`
+        // implementation of a type stored in a thread
+        // local field may still try to access the global
+        // statics on drop, so we need to take extra care
+        // that the global statics outlive the thread locals.
+        let statics = execution(|execution| {
+            let thread = execution.threads.active_id();
+
+            trace!(?thread, "thread_done: drop statics from the main thread");
+
+            execution.lazy_statics.drop()
+        });
+
+        // Drop statics outside of the execution context
+        drop(statics);
+    }
+
     execution(|execution| {
         let thread = execution.threads.active_id();
 

tests/thread_local.rs

@@ -107,3 +107,70 @@ fn drop() {
     // should also be dropped.
     assert_eq!(DROPS.load(Ordering::Acquire), 3);
 }
+
+/// Test that TLS destructors are allowed to access global statics
+/// when the TLS type is dropped.
+///
+/// This is a regression test for:
+/// <https://github.com/tokio-rs/loom/issues/152>
+#[test]
+fn lazy_static() {
+    loom::lazy_static! {
+        static ref ID: usize = 0x42;
+    }
+
+    loom::thread_local! {
+        static BAR: Bar = Bar;
+    }
+
+    struct Bar;
+
+    impl Drop for Bar {
+        fn drop(&mut self) {
+            let _ = &*ID;
+        }
+    }
+
+    loom::model(|| {
+        BAR.with(|_| ());
+    });
+}
+
+/// When a thread panics it runs the TLS destructors, which
+/// in turn may try to access a global static. If the drop
+/// order of TLS fields and global statics is switched, then
+/// this will trigger a panic from the TLS destructor, too.
+/// This causes a panic inside another panic, which will lead
+/// to loom triggering a segfault. This should not happen,
+/// because it should be allowed for TLS destructors to always
+/// access a global static.
+///
+/// This is a regression test for a slight varation of
+/// <https://github.com/tokio-rs/loom/issues/152>.
+#[test]
+#[should_panic(expected = "loom should not panic inside another panic")]
+fn lazy_static_panic() {
+    loom::lazy_static! {
+        static ref ID: usize = 0x42;
+    }
+
+    loom::thread_local! {
+        static BAR: Bar = Bar;
+    }
+
+    struct Bar;
+
+    impl Drop for Bar {
+        fn drop(&mut self) {
+            let _ = &*ID;
+        }
+    }
+
+    loom::model(|| {
+        // initialize the TLS destructor:
+        BAR.with(|_| ());
+        println!("about to panic");
+        // intentionally panic:
+        panic!("loom should not panic inside another panic");
+    });
+}