diff --git a/rust/nix-expr/src/eval_state.rs b/rust/nix-expr/src/eval_state.rs index 61cfde8..cfa39a3 100644 --- a/rust/nix-expr/src/eval_state.rs +++ b/rust/nix-expr/src/eval_state.rs @@ -594,7 +594,8 @@ mod tests { fn weak_ref_gone() { gc_registering_current_thread(|| { let weak = { - let store = Store::open("auto", HashMap::new()).unwrap(); + // Use a slightly different URL which is unique in the test suite, to bypass the global store cache + let store = Store::open("auto?foo=bar", HashMap::new()).unwrap(); let es = EvalState::new(store, []).unwrap(); es.weak_ref() }; diff --git a/rust/nix-store/src/store.rs b/rust/nix-store/src/store.rs index 54fda9c..87295d2 100644 --- a/rust/nix-store/src/store.rs +++ b/rust/nix-store/src/store.rs @@ -1,13 +1,14 @@ -use anyhow::{bail, Result}; +use anyhow::{bail, Error, Result}; use lazy_static::lazy_static; use nix_c_raw as raw; use nix_util::context::Context; use nix_util::string_return::{callback_get_result_string, callback_get_result_string_data}; use nix_util::{check_call, result_string_init}; +use std::collections::HashMap; use std::ffi::{c_char, CString}; use std::ptr::null_mut; use std::ptr::NonNull; -use std::sync::{Arc, Weak}; +use std::sync::{Arc, Mutex, Weak}; /* TODO make Nix itself thread safe */ lazy_static! { @@ -33,6 +34,8 @@ impl Drop for StoreRef { } } unsafe impl Send for StoreRef {} +/// Unlike pointers in general, operations on raw::Store are thread safe and it is therefore safe to share them between threads. +unsafe impl Sync for StoreRef {} /// A [Weak] reference to a store. pub struct StoreWeak { @@ -50,6 +53,13 @@ impl StoreWeak { } } +/// Protects against https://github.com/NixOS/nix/issues/11979 (unless different parameters are passed, in which case it's up to luck, but you do get your own parameters as you asked for). +type StoreCacheMap = HashMap<(String, Vec<(String, String)>), StoreWeak>; + +lazy_static! { + static ref STORE_CACHE: Arc> = Arc::new(Mutex::new(HashMap::new())); +} + pub struct Store { inner: Arc, /* An error context to reuse. This way we don't have to allocate them for each store operation. */ @@ -59,6 +69,41 @@ impl Store { pub fn open<'a, 'b>( url: &str, params: impl IntoIterator, + ) -> Result { + let params = params + .into_iter() + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect::>(); + let params2 = params.clone(); + let mut store_cache = STORE_CACHE + .lock() + .map_err(|_| Error::msg("Failed to lock store cache. This should never happen."))?; + match store_cache.entry((url.to_string(), params)) { + std::collections::hash_map::Entry::Occupied(mut e) => { + if let Some(store) = e.get().upgrade() { + Ok(store) + } else { + let store = Self::open_uncached( + url, + params2.iter().map(|(k, v)| (k.as_str(), v.as_str())), + )?; + e.insert(store.weak_ref()); + Ok(store) + } + } + std::collections::hash_map::Entry::Vacant(e) => { + let store = Self::open_uncached( + url, + params2.iter().map(|(k, v)| (k.as_str(), v.as_str())), + )?; + e.insert(store.weak_ref()); + Ok(store) + } + } + } + fn open_uncached<'a, 'b>( + url: &str, + params: impl IntoIterator, ) -> Result { let x = INIT.as_ref(); match x { @@ -191,7 +236,9 @@ mod tests { #[test] fn weak_ref_gone() { let weak = { - let store = Store::open("auto", HashMap::new()).unwrap(); + // Concurrent tests calling Store::open will keep the weak reference to auto alive, + // so for this test we need to bypass the global cache. + let store = Store::open_uncached("auto", HashMap::new()).unwrap(); store.weak_ref() }; assert!(weak.upgrade().is_none());