introduce fork, which yields a frozen handle

src/lib.rs

@@ -20,7 +20,7 @@ use derive_new::new;
 use std::fmt::{self, Debug};
 use std::hash::Hash;
 
-pub use crate::runtime::RevisionGuard;
+pub use crate::runtime::Frozen;
 pub use crate::runtime::Runtime;
 pub use crate::runtime::RuntimeId;
 
@@ -177,12 +177,52 @@ impl Default for SweepStrategy {
 /// parallel execution, but for it to work, your query key/value types
 /// must also be `Send`, as must any additional data in your database.
 pub trait ParallelDatabase: Database + Send {
-    /// Creates a copy of this database destined for another
-    /// thread. See also `Runtime::fork`.
+    /// Creates a second handle to the database that holds the
+    /// database fixed at a particular revision. So long as this
+    /// "frozen" handle exists, any attempt to [`set`] an input will
+    /// block.
     ///
-    /// **Warning.** This second handle is intended to be used from a
+    /// [`set`]: struct.QueryTable.html#method.set
+    ///
+    /// This is the method you are meant to use most of the time in a
+    /// parallel setting where modifications may arise asynchronously
+    /// (e.g., a language server). In this context, it is common to
+    /// wish to "fork off" a copy of the database performing some
+    /// series of queries in parallel and arranging the results. Using
+    /// this method for that purpose ensures that those queries will
+    /// see a consistent view of the database (it is also advisable
+    /// for those queries to use the [`is_current_revision_canceled`]
+    /// method to check for cancellation).
+    ///
+    /// [`is_current_revision_canceled`]: struct.Runtime.html#method.is_current_revision_canceled
+    ///
+    /// # Deadlock warning
+    ///
+    /// This second handle is intended to be used from a separate
+    /// thread. Using two database handles from the **same thread**
+    /// can lead to deadlock.
+    fn fork(&self) -> Frozen<Self> {
+        Frozen::new(self)
+    }
+
+    /// Creates another handle to this database destined for another
+    /// thread. You are meant to implement this by cloning a second
+    /// handle to all all the state in the database; to clone the
+    /// salsa runtime, use the [`Runtime::fork_mut`] method.
+    ///
+    /// Note to users: you only need this method if you plan to be
+    /// setting inputs on the other thread. Otherwise, it is preferred
+    /// to use [`fork`], which will given back a frozen database fixed
+    /// at the current revision.
+    ///
+    /// # Deadlock warning
+    ///
+    /// This second handle is intended to be used from a
     /// separate thread. Using two database handles from the **same
     /// thread** can lead to deadlock.
+    ///
+    /// [`Runtime::fork_mut`]: https://docs.rs/salsa/0.7.0/salsa/struct.Runtime.html#method.fork_mut
+    /// [`fork`]: trait.ParallelDatabase.html#tymethod.fork
     fn fork_mut(&self) -> Self;
 }
 

src/runtime.rs

@@ -1,5 +1,5 @@
-use crate::{Database, Event, EventKind, SweepStrategy};
-use lock_api::RawRwLock;
+use crate::{Database, Event, EventKind, ParallelDatabase, SweepStrategy};
+use lock_api::{RawRwLock, RawRwLockRecursive};
 use log::debug;
 use parking_lot::{Mutex, RwLock, RwLockReadGuard};
 use rustc_hash::{FxHashMap, FxHasher};
@@ -63,8 +63,8 @@ where
         &self.shared_state.storage
     }
 
-    /// As with `Database::fork`, creates a second copy of the runtime
-    /// meant to be used from another thread.
+    /// As with `Database::fork_mut`, creates a second handle to this
+    /// runtime meant to be used from another thread.
     ///
     /// **Warning.** This second handle is intended to be used from a
     /// separate thread. Using two database handles from the **same
@@ -123,30 +123,6 @@ where
         }
     }
 
-    /// Locks the current revision and returns a guard object that --
-    /// when dropped -- will unlock it. While a revision is locked,
-    /// queries can execute as normal but calls to `set` will block
-    /// (note that calls to `set` *do* set the cancellation flag,
-    /// which you can can check with
-    /// `is_current_revision_canceled`). The intention is that you can
-    /// lock the revision and then do multiple queries, thus
-    /// guaranteeing that all of those queries execute against a
-    /// consistent "view" of the database.
-    ///
-    /// Note that, unlike most RAII guards, the guard returned by this
-    /// method does not borrow the database or the runtime
-    /// (internally, it uses an `Arc` handle). This means it can be
-    /// sent to other threads without a problem -- the lock persists
-    /// as long as the guard has not yet been dropped.
-    ///
-    /// ### Deadlock warning
-    ///
-    /// If you invoke `lock_revision` and then, from the same thread,
-    /// call `set` on some input, you will get a deadlock.
-    pub fn lock_revision(&self) -> RevisionGuard<DB> {
-        RevisionGuard::new(&self.shared_state)
-    }
-
     /// The unique identifier attached to this `SalsaRuntime`. Each
     /// forked runtime has a distinct identifier.
     #[inline]
@@ -447,38 +423,6 @@ impl<'db, DB: Database> Drop for QueryGuard<'db, DB> {
     }
 }
 
-/// The guard returned by `lock_revision`. Once this guard is dropped,
-/// the revision will be unlocked, and calls to `set` can proceed.
-pub struct RevisionGuard<DB: Database> {
-    shared_state: Arc<SharedState<DB>>,
-}
-
-impl<DB: Database> RevisionGuard<DB> {
-    /// Creates a new revision guard, acquiring the query read-lock in the process.
-    fn new(shared_state: &Arc<SharedState<DB>>) -> Self {
-        // Acquire the read-lock without using RAII. This requires the
-        // unsafe keyword because, if we were to unlock the lock this way,
-        // we would screw up other people using the safe APIs.
-        unsafe {
-            shared_state.query_lock.raw().lock_shared();
-        }
-
-        Self {
-            shared_state: shared_state.clone(),
-        }
-    }
-}
-
-impl<DB: Database> Drop for RevisionGuard<DB> {
-    fn drop(&mut self) {
-        // Release our read-lock without using RAII. As in `new`
-        // above, this requires the unsafe keyword.
-        unsafe {
-            self.shared_state.query_lock.raw().unlock_shared();
-        }
-    }
-}
-
 struct ActiveQuery<DB: Database> {
     /// What query is executing
     descriptor: DB::QueryDescriptor,
@@ -650,3 +594,120 @@ impl<DB: Database> DependencyGraph<DB> {
         }
     }
 }
+
+/// The `Frozen` struct indicates a database handle which is locked at
+/// a particular revision: any attempt to set the value of an input
+/// (e.g., from another database handle) will block until the `Frozen`
+/// database is dropped.
+///
+/// # Deadlock warning
+///
+/// Since attempts to set inputs are blocked until the `Frozen<DB>` is
+/// dropped, this implies that any attempt to set an input *using* the
+/// `Frozen<DB>` will deadlock instantly (because it will block the
+/// thread that owns the `Frozen<DB>` and thus not permit the
+/// `Frozen<DB>` to be dropped). In the future, we plan to refactor
+/// the API to make such "instant deadlocks" impossible.
+pub struct Frozen<DB>
+where
+    DB: ParallelDatabase,
+{
+    shared_state: Arc<SharedState<DB>>,
+    db: DB,
+}
+
+impl<DB> Frozen<DB>
+where
+    DB: ParallelDatabase,
+{
+    /// Creates and returns a frozen handle to `source_db`.
+    pub(crate) fn new(source_db: &DB) -> Self {
+        let source_runtime = source_db.salsa_runtime();
+
+        // Subtle point: if the source database is already executing a
+        // query, then we want to use a "recursive read" lock.  Using
+        // an ordinary read lock [may deadlock], since it could wind
+        // up blocking indefinitely if there is a pending write (thus
+        // preventing our caller from releasing their read lock, which
+        // in turn ensures that the pending write will never
+        // complete).
+        //
+        // We could just use recursive reads *always*, but that could
+        // lead to starvation in the case where you have various
+        // threads invoking get and set willy nilly. Not sure how
+        // important it is to ensure that case works -- it's not a
+        // recommended pattern -- but it seems (for now at least) easy
+        // enough to do so.
+        //
+        // [may deadlock]: https://docs.rs/lock_api/0.1.3/lock_api/struct.RwLock.html#method.read
+        let use_recursive_lock = source_runtime.query_in_progress();
+
+        // Fork off a new database for us to use.
+        let our_db = source_db.fork_mut();
+        let our_runtime = our_db.salsa_runtime();
+
+        // Set the `query_in_progress` flag permanently true for our
+        // database to prevent queries that execute against it from
+        // acquiring the read lock.
+        {
+            let mut local_state = our_runtime.local_state.borrow_mut();
+            if local_state.query_in_progress {
+                panic!("cannot use `Frozen::new` with a query in progress")
+            }
+            local_state.query_in_progress = true;
+        }
+
+        // OK, this is paranoia. *Technically speaking*, we don't
+        // control the DB type, so it may "yield up" different
+        // runtimes at different points. So we will save the
+        // shared-state that we are operating on to be sure it does
+        // not. This way, we are sure that we are invoking the "unpaired"
+        // lock and unlock operations on the same lock.
+        //
+        // (Note that -- if people are being wacky -- we might be
+        // changing the `query_in_progress` flag on the wrong local
+        // state. That I believe can only trigger *deadlock* so I'm
+        // not as worried, but perhaps I should be.)
+        let shared_state = our_runtime.shared_state.clone();
+
+        // Acquire the read-lock without using RAII. This requires the
+        // unsafe keyword because, if we were to unlock the lock this
+        // way, we would screw up other people using the safe APIs.
+        unsafe {
+            if use_recursive_lock {
+                shared_state.query_lock.raw().lock_shared_recursive();
+            } else {
+                shared_state.query_lock.raw().lock_shared();
+            }
+        }
+
+        Frozen {
+            shared_state,
+            db: our_db,
+        }
+    }
+}
+
+impl<DB> std::ops::Deref for Frozen<DB>
+where
+    DB: ParallelDatabase,
+{
+    type Target = DB;
+
+    fn deref(&self) -> &DB {
+        &self.db
+    }
+}
+
+impl<DB> Drop for Frozen<DB>
+where
+    DB: ParallelDatabase,
+{
+    fn drop(&mut self) {
+        // Release our read-lock without using RAII. As documented in
+        // `Frozen::new` above, this requires the unsafe keyword.
+        unsafe {
+            self.shared_state.query_lock.raw().unlock_shared();
+        }
+    }
+}

tests/parallel/frozen.rs

@@ -14,9 +14,8 @@ fn in_par_get_set_cancellation() {
 
     let signal = Arc::new(Signal::default());
 
-    let lock = db.salsa_runtime().lock_revision();
     let thread1 = std::thread::spawn({
-        let db = db.fork_mut();
+        let db = db.fork();
         let signal = signal.clone();
         move || {
             // Check that cancellation flag is not yet set, because
@@ -35,11 +34,8 @@ fn in_par_get_set_cancellation() {
             // see 1 here.
             let v = db.input('a');
 
-            // Release the lock.
-            std::mem::drop(lock);
-
-            // This could come before or after the `set` in the other
-            // thread.
+            // Since this is a forked database, we are in a consistent
+            // revision, so this must yield the same value.
             let w = db.input('a');
 
             (v, w)
@@ -61,11 +57,9 @@ fn in_par_get_set_cancellation() {
         }
     });
 
-    // The first read is done with the revision lock, so it *must* see
-    // `1`; the second read could see either `1` or `2`.
     let (a, b) = thread1.join().unwrap();
     assert_eq!(a, 1);
-    assert!(b == 1 || b == 2, "saw unexpected value for b: {}", b);
+    assert_eq!(b, 1);
 
     let c = thread2.join().unwrap();
     assert_eq!(c, 2);

tests/parallel/main.rs

@@ -1,9 +1,9 @@
 mod setup;
 
 mod cancellation;
+mod frozen;
 mod independent;
 mod race;
-mod revision_lock;
 mod signal;
-mod true_parallel;
 mod stress;
+mod true_parallel;