Merge pull request #474 from Chronostasys/master

fix: wrong condvar usage

components/salsa-2022/src/storage.rs

@@ -48,6 +48,9 @@ struct Shared<DB: HasJars> {
     /// Conditional variable that is used to coordinate cancellation.
     /// When the main thread writes to the database, it blocks until each of the snapshots can be cancelled.
     cvar: Arc<Condvar>,
+
+    /// Mutex that is used to protect the `jars` field when waiting for snapshots to be dropped.
+    noti_lock: Arc<parking_lot::Mutex<()>>,
 }
 
 // ANCHOR: default
@@ -62,6 +65,7 @@ where
             shared: Shared {
                 jars: Some(Arc::from(jars)),
                 cvar: Arc::new(Default::default()),
+                noti_lock: Arc::new(parking_lot::Mutex::new(())),
             },
             routes: Arc::new(routes),
             runtime: Runtime::default(),
@@ -135,19 +139,23 @@ where
         loop {
             self.runtime.set_cancellation_flag();
 
+            // Acquire lock before we check if we have unique access to the jars.
+            // If we do not yet have unique access, we will go to sleep and wait for
+            // the snapshots to be dropped, which will signal the cond var associated
+            // with this lock.
+            //
+            // NB: We have to acquire the lock first to ensure that we can check for
+            // unique access and go to sleep waiting on the condvar atomically,
+            // as described in PR #474.
+            let mut guard = self.shared.noti_lock.lock();
             // If we have unique access to the jars, we are done.
             if Arc::get_mut(self.shared.jars.as_mut().unwrap()).is_some() {
                 return;
             }
 
             // Otherwise, wait until some other storage entities have dropped.
-            // We create a mutex here because the cvar api requires it, but we
-            // don't really need one as the data being protected is actually
-            // the jars above.
             //
             // The cvar `self.shared.cvar` is notified by the `Drop` impl.
-            let mutex = parking_lot::Mutex::new(());
-            let mut guard = mutex.lock();
             self.shared.cvar.wait(&mut guard);
         }
     }
@@ -167,6 +175,7 @@ where
         Self {
             jars: self.jars.clone(),
             cvar: self.cvar.clone(),
+            noti_lock: self.noti_lock.clone(),
         }
     }
 }
@@ -178,6 +187,7 @@ where
     fn drop(&mut self) {
         // Drop the Arc reference before the cvar is notified,
         // since other threads are sleeping, waiting for it to reach 1.
+        let _guard = self.shared.noti_lock.lock();
         drop(self.shared.jars.take());
         self.shared.cvar.notify_all();
     }