Merge pull request #675 from Veykril/veykril/push-xsrukmlqwrlm

Introduce `RefUnwindSafe` `StorageHandle`

src/storage.rs

@@ -1,3 +1,4 @@
+//! Public API facades for the implementation details of [`Zalsa`] and [`ZalsaLocal`].
 use std::{marker::PhantomData, panic::RefUnwindSafe, sync::Arc};
 
 use parking_lot::{Condvar, Mutex};
@@ -8,6 +9,55 @@ use crate::{
     Database, Event, EventKind,
 };
 
+/// A handle to non-local database state.
+pub struct StorageHandle<Db> {
+    // Note: Drop order is important, zalsa_impl needs to drop before coordinate
+    /// Reference to the database.
+    zalsa_impl: Arc<Zalsa>,
+
+    // Note: Drop order is important, coordinate needs to drop after zalsa_impl
+    /// Coordination data for cancellation of other handles when `zalsa_mut` is called.
+    /// This could be stored in Zalsa but it makes things marginally cleaner to keep it separate.
+    coordinate: CoordinateDrop,
+
+    /// We store references to `Db`
+    phantom: PhantomData<fn() -> Db>,
+}
+
+impl<Db> Clone for StorageHandle<Db> {
+    fn clone(&self) -> Self {
+        *self.coordinate.clones.lock() += 1;
+
+        Self {
+            zalsa_impl: self.zalsa_impl.clone(),
+            coordinate: CoordinateDrop(Arc::clone(&self.coordinate)),
+            phantom: PhantomData,
+        }
+    }
+}
+
+impl<Db: Database> Default for StorageHandle<Db> {
+    fn default() -> Self {
+        Self {
+            zalsa_impl: Arc::new(Zalsa::new::<Db>()),
+            coordinate: CoordinateDrop(Arc::new(Coordinate {
+                clones: Mutex::new(1),
+                cvar: Default::default(),
+            })),
+            phantom: PhantomData,
+        }
+    }
+}
+
+impl<Db> StorageHandle<Db> {
+    pub fn into_storage(self) -> Storage<Db> {
+        Storage {
+            handle: self,
+            zalsa_local: ZalsaLocal::new(),
+        }
+    }
+}
+
 /// Access the "storage" of a Salsa database: this is an internal plumbing trait
 /// automatically implemented by `#[salsa::db]` applied to a struct.
 ///
@@ -20,76 +70,77 @@ pub unsafe trait HasStorage: Database + Clone + Sized {
     fn storage_mut(&mut self) -> &mut Storage<Self>;
 }
 
-/// Concrete implementation of the [`Database`][] trait.
-/// Takes an optional type parameter `U` that allows you to thread your own data.
-pub struct Storage<Db: Database> {
-    // Note: Drop order is important, zalsa_impl needs to drop before coordinate
-    /// Reference to the database.
-    zalsa_impl: Arc<Zalsa>,
-
-    // Note: Drop order is important, coordinate needs to drop after zalsa_impl
-    /// Coordination data for cancellation of other handles when `zalsa_mut` is called.
-    /// This could be stored in Zalsa but it makes things marginally cleaner to keep it separate.
-    coordinate: CoordinateDrop,
+/// Concrete implementation of the [`Database`] trait with local state that can be used to drive computations.
+pub struct Storage<Db> {
+    handle: StorageHandle<Db>,
 
     /// Per-thread state
     zalsa_local: zalsa_local::ZalsaLocal,
-
-    /// We store references to `Db`
-    phantom: PhantomData<fn() -> Db>,
 }
+
 struct Coordinate {
     /// Counter of the number of clones of actor. Begins at 1.
     /// Incremented when cloned, decremented when dropped.
     clones: Mutex<usize>,
     cvar: Condvar,
 }
 
+// We cannot panic while holding a lock to `clones: Mutex<usize>` and therefore we cannot enter an
+// inconsistent state.
+impl RefUnwindSafe for Coordinate {}
+
 impl<Db: Database> Default for Storage<Db> {
     fn default() -> Self {
         Self {
-            zalsa_impl: Arc::new(Zalsa::new::<Db>()),
-            coordinate: CoordinateDrop(Arc::new(Coordinate {
-                clones: Mutex::new(1),
-                cvar: Default::default(),
-            })),
+            handle: StorageHandle::default(),
             zalsa_local: ZalsaLocal::new(),
-            phantom: PhantomData,
         }
     }
 }
 
 impl<Db: Database> Storage<Db> {
+    /// Convert this instance of [`Storage`] into a [`StorageHandle`].
+    ///
+    /// This will discard the local state of this [`Storage`], thereby returning a value that
+    /// is both [`Sync`] and [`std::panic::UnwindSafe`].
+    pub fn into_zalsa_handle(self) -> StorageHandle<Db> {
+        let Storage {
+            handle,
+            zalsa_local: _,
+        } = self;
+        handle
+    }
+
     // ANCHOR: cancel_other_workers
     /// Sets cancellation flag and blocks until all other workers with access
     /// to this storage have completed.
     ///
     /// This could deadlock if there is a single worker with two handles to the
     /// same database!
     fn cancel_others(&self, db: &Db) {
-        self.zalsa_impl.set_cancellation_flag();
+        self.handle.zalsa_impl.set_cancellation_flag();
 
         db.salsa_event(&|| Event::new(EventKind::DidSetCancellationFlag));
 
-        let mut clones = self.coordinate.clones.lock();
+        let mut clones = self.handle.coordinate.clones.lock();
         while *clones != 1 {
-            self.coordinate.cvar.wait(&mut clones);
+            self.handle.coordinate.cvar.wait(&mut clones);
         }
     }
     // ANCHOR_END: cancel_other_workers
 }
 
 unsafe impl<T: HasStorage> ZalsaDatabase for T {
     fn zalsa(&self) -> &Zalsa {
-        &self.storage().zalsa_impl
+        &self.storage().handle.zalsa_impl
     }
 
     fn zalsa_mut(&mut self) -> &mut Zalsa {
         self.storage().cancel_others(self);
 
         let storage = self.storage_mut();
         // The ref count on the `Arc` should now be 1
-        let zalsa_mut = Arc::get_mut(&mut storage.zalsa_impl).unwrap();
+        let zalsa_mut = Arc::get_mut(&mut storage.handle.zalsa_impl).unwrap();
         zalsa_mut.new_revision();
         zalsa_mut
     }
@@ -103,17 +154,11 @@ unsafe impl<T: HasStorage> ZalsaDatabase for T {
     }
 }
 
-impl<Db: Database> RefUnwindSafe for Storage<Db> {}
-
 impl<Db: Database> Clone for Storage<Db> {
     fn clone(&self) -> Self {
-        *self.coordinate.clones.lock() += 1;
-
         Self {
-            zalsa_impl: self.zalsa_impl.clone(),
-            coordinate: CoordinateDrop(Arc::clone(&self.coordinate)),
+            handle: self.handle.clone(),
             zalsa_local: ZalsaLocal::new(),
-            phantom: PhantomData,
         }
     }
 }

src/zalsa.rs

@@ -3,6 +3,7 @@ use parking_lot::{Mutex, RwLock};
 use rustc_hash::FxHashMap;
 use std::any::{Any, TypeId};
 use std::marker::PhantomData;
+use std::panic::RefUnwindSafe;
 use std::thread::ThreadId;
 
 use crate::cycle::CycleRecoveryStrategy;
@@ -149,6 +150,13 @@ pub struct Zalsa {
     runtime: Runtime,
 }
 
+/// All fields on Zalsa are locked behind [`Mutex`]es and [`RwLock`]s and cannot enter
+/// inconsistent states. The contents of said fields are largely ID mappings, with the exception
+/// of [`Runtime::dependency_graph`]. However, [`Runtime::dependency_graph`] does not
+/// invoke any queries and as such there will be no panic from code downstream of Salsa. It can only
+/// panic if an assertion inside of Salsa fails.
+impl RefUnwindSafe for Zalsa {}
+
 impl Zalsa {
     pub(crate) fn new<Db: Database>() -> Self {
         Self {

tests/check_auto_traits.rs

@@ -0,0 +1,50 @@
+//! Test that auto trait impls exist as expected.
+
+use std::panic::UnwindSafe;
+
+use salsa::Database;
+use test_log::test;
+
+#[salsa::input]
+struct MyInput {
+    field: String,
+}
+
+#[salsa::tracked]
+struct MyTracked<'db> {
+    field: MyInterned<'db>,
+}
+
+#[salsa::interned]
+struct MyInterned<'db> {
+    field: String,
+}
+
+#[salsa::tracked]
+fn test(db: &dyn Database, input: MyInput) {
+    let input = is_send(is_sync(input));
+    let interned = is_send(is_sync(MyInterned::new(db, input.field(db).clone())));
+    let _tracked_struct = is_send(is_sync(MyTracked::new(db, interned)));
+}
+
+fn is_send<T: Send>(t: T) -> T {
+    t
+}
+
+fn is_sync<T: Send>(t: T) -> T {
+    t
+}
+
+fn is_unwind_safe<T: UnwindSafe>(t: T) -> T {
+    t
+}
+
+#[test]
+fn execute() {
+    let db = is_send(salsa::DatabaseImpl::new());
+    let _handle = is_send(is_sync(is_unwind_safe(
+        db.storage().clone().into_zalsa_handle(),
+    )));
+    let input = MyInput::new(&db, "Hello".to_string());
+    test(&db, input);
+}