Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: wrong condvar usage #474

Merged
merged 3 commits into from
Apr 2, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions components/salsa-2022/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(),
Expand Down Expand Up @@ -135,19 +139,16 @@ where
loop {
self.runtime.set_cancellation_flag();

// jars need to be protected by a lock to avoid deadlocks.
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
let mut guard = self.shared.noti_lock.lock();
Chronostasys marked this conversation as resolved.
Show resolved Hide resolved
// 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);
}
}
Expand All @@ -167,6 +168,7 @@ where
Self {
jars: self.jars.clone(),
cvar: self.cvar.clone(),
noti_lock: self.noti_lock.clone(),
}
}
}
Expand All @@ -178,6 +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();
drop(self.shared.jars.take());
self.shared.cvar.notify_all();
}
Expand Down