From 864344addbc0ac3f38ea5e600ca7d8564ffab9c9 Mon Sep 17 00:00:00 2001 From: Mona Lisa <1769712655@qq.com> Date: Fri, 15 Mar 2024 22:03:15 +0800 Subject: [PATCH 1/3] fix: wrong condvar usage --- components/salsa-2022/src/storage.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index 49b0a50a..b36d8477 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -31,6 +31,7 @@ pub struct Storage { /// The runtime for this particular salsa database handle. /// Each handle gets its own runtime, but the runtimes have shared state between them. runtime: Runtime, + } /// Data shared between all threads. @@ -48,6 +49,9 @@ struct Shared { /// 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, + + /// 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 +66,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 +140,16 @@ where loop { self.runtime.set_cancellation_flag(); + // jars need to be protected by a lock to avoid deadlocks. + 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 +169,7 @@ where Self { jars: self.jars.clone(), cvar: self.cvar.clone(), + noti_lock: self.noti_lock.clone() } } } @@ -178,6 +181,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(); } From 998bcf606a0bef361ad6d9c37a35f3db83f7c94d Mon Sep 17 00:00:00 2001 From: Mona Lisa <1769712655@qq.com> Date: Fri, 15 Mar 2024 23:01:30 +0800 Subject: [PATCH 2/3] chore: fmt --- components/salsa-2022/src/storage.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index b36d8477..bbc09799 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -31,7 +31,6 @@ pub struct Storage { /// The runtime for this particular salsa database handle. /// Each handle gets its own runtime, but the runtimes have shared state between them. runtime: Runtime, - } /// Data shared between all threads. @@ -51,7 +50,7 @@ struct Shared { cvar: Arc, /// Mutex that is used to protect the `jars` field when waiting for snapshots to be dropped. - noti_lock:Arc< parking_lot::Mutex<()>> + noti_lock: Arc>, } // ANCHOR: default @@ -66,7 +65,7 @@ where shared: Shared { jars: Some(Arc::from(jars)), cvar: Arc::new(Default::default()), - noti_lock:Arc::new( parking_lot::Mutex::new(())) + noti_lock: Arc::new(parking_lot::Mutex::new(())), }, routes: Arc::new(routes), runtime: Runtime::default(), @@ -141,7 +140,7 @@ where self.runtime.set_cancellation_flag(); // jars need to be protected by a lock to avoid deadlocks. - let mut guard =self.shared.noti_lock.lock(); + 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; @@ -169,7 +168,7 @@ where Self { jars: self.jars.clone(), cvar: self.cvar.clone(), - noti_lock: self.noti_lock.clone() + noti_lock: self.noti_lock.clone(), } } } @@ -181,7 +180,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(); + let _guard = self.shared.noti_lock.lock(); drop(self.shared.jars.take()); self.shared.cvar.notify_all(); } From 9deaf3e149a66966158699fa1ce7958243ddb45f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 2 Apr 2024 06:15:43 -0400 Subject: [PATCH 3/3] Update components/salsa-2022/src/storage.rs --- components/salsa-2022/src/storage.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index bbc09799..df92ed78 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -139,7 +139,14 @@ where loop { self.runtime.set_cancellation_flag(); - // jars need to be protected by a lock to avoid deadlocks. + // 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() {