diff --git a/src/lib.rs b/src/lib.rs index 65ae936c..faaa9eb2 100644 --- a/src/lib.rs +++ b/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 { + 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; } diff --git a/src/runtime.rs b/src/runtime.rs index 31cde491..3aa8003a 100644 --- a/src/runtime.rs +++ b/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 { - 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 { - shared_state: Arc>, -} - -impl RevisionGuard { - /// Creates a new revision guard, acquiring the query read-lock in the process. - fn new(shared_state: &Arc>) -> 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 Drop for RevisionGuard { - 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 { /// What query is executing descriptor: DB::QueryDescriptor, @@ -650,3 +594,120 @@ impl DependencyGraph { } } } + +/// 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` is +/// dropped, this implies that any attempt to set an input *using* the +/// `Frozen` will deadlock instantly (because it will block the +/// thread that owns the `Frozen` and thus not permit the +/// `Frozen` to be dropped). In the future, we plan to refactor +/// the API to make such "instant deadlocks" impossible. +pub struct Frozen +where + DB: ParallelDatabase, +{ + shared_state: Arc>, + db: DB, +} + +impl Frozen +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 std::ops::Deref for Frozen +where + DB: ParallelDatabase, +{ + type Target = DB; + + fn deref(&self) -> &DB { + &self.db + } +} + +impl Drop for Frozen +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(); + } + } +} diff --git a/tests/parallel/revision_lock.rs b/tests/parallel/frozen.rs similarity index 80% rename from tests/parallel/revision_lock.rs rename to tests/parallel/frozen.rs index d241953f..3b9bf08a 100644 --- a/tests/parallel/revision_lock.rs +++ b/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); diff --git a/tests/parallel/main.rs b/tests/parallel/main.rs index 7b8823e1..9c6e5360 100644 --- a/tests/parallel/main.rs +++ b/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;