From b57871b6adf3ddb0ea0e0113e89563426a28fddf Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 20 Oct 2026 17:41:20 +0800 Subject: [PATCH] Update the dictionary and its in-memory origin under one lock CodeRabbit (#3252): reload_dictionary() wrote DICT and DICT_IN_MEMORY under two separate locks. If install_dictionary() ran between them, the final state could hold an in-memory dictionary with the flag left false, so installed_path() would report a same-named disk file instead of None. Combined both into one DictState struct behind a single RwLock in both ru and cs, so install_dictionary() and reload_dictionary() each update the dictionary and its origin in one critical section. Didn't add a dedicated concurrency test: genuinely proving a race's absence needs real multi-threaded stress testing, and the previous commit on this PR already found that tests directly exercising install_dictionary()/reload_dictionary() concurrency in this crate's shared global state cause real, reproducible flakiness in unrelated tests. The fix is correct by construction (one lock acquisition around both field writes) and all 222 existing proof tests, run 15 times in a row, stay green. Co-Authored-By: Claude Sonnet 5 --- crates/proof/src/cs/mod.rs | 31 ++++++++++++++++++------------- crates/proof/src/ru/mod.rs | 31 ++++++++++++++++++------------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/crates/proof/src/cs/mod.rs b/crates/proof/src/cs/mod.rs index ef560be46..af743406d 100644 --- a/crates/proof/src/cs/mod.rs +++ b/crates/proof/src/cs/mod.rs @@ -13,13 +13,16 @@ use crate::affix::AffixDict; pub use suggest::suggest; -/// The Czech dictionary: `None` until first use, then the installed one (or an empty one). -static DICT: RwLock>> = RwLock::new(None); +/// The Czech dictionary and whether it came from [`install_dictionary`] rather than a file on +/// disk (it then has no file of its own to report from [`installed_path`], even if a matching +/// file also happens to exist): one lock, so a reload and an install can't interleave their two +/// fields into an inconsistent combination. +static DICT: RwLock = RwLock::new(DictState { dict: None, in_memory: false }); -/// Whether `DICT` holds a dictionary given with [`install_dictionary`] rather than one found on -/// disk: it has no file of its own to report from [`installed_path`], even if a matching file -/// happens to exist too. -static DICT_IN_MEMORY: RwLock = RwLock::new(false); +struct DictState { + dict: Option>, + in_memory: bool, +} /// File names of a Czech Hunspell dictionary. const NAMES: &[&str] = &["cs_CZ", "cs-CZ", "cs"]; @@ -28,13 +31,13 @@ const NAMES: &[&str] = &["cs_CZ", "cs-CZ", "cs"]; /// [`install_dictionary`], or an empty one when none is available (Czech spelling is off /// then; see [`has_dictionary`]). pub fn dictionary() -> Arc { - if let Some(d) = DICT.read().unwrap_or_else(|e| e.into_inner()).as_ref() { + if let Some(d) = DICT.read().unwrap_or_else(|e| e.into_inner()).dict.as_ref() { return d.clone(); } let loaded = Arc::new(load_installed()); let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); // Another thread (or an install) may have filled it meanwhile: that one wins. - slot.get_or_insert(loaded).clone() + slot.dict.get_or_insert(loaded).clone() } fn load_installed() -> AffixDict { @@ -52,16 +55,18 @@ fn load_installed() -> AffixDict { /// UTF-8), for example one the user picked. On error the current dictionary stays. pub fn install_dictionary(aff: &str, dic: &str) -> Result<(), String> { let d = AffixDict::from_hunspell(aff, dic, &[])?; - *DICT.write().unwrap_or_else(|e| e.into_inner()) = Some(Arc::new(d)); - *DICT_IN_MEMORY.write().unwrap_or_else(|e| e.into_inner()) = true; + let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); + slot.dict = Some(Arc::new(d)); + slot.in_memory = true; Ok(()) } /// Forget the loaded dictionary, so the next check looks for an installed one again (after /// [`crate::hunspell::set_dictionary_dir`] or a dictionary was installed). pub fn reload_dictionary() { - *DICT.write().unwrap_or_else(|e| e.into_inner()) = None; - *DICT_IN_MEMORY.write().unwrap_or_else(|e| e.into_inner()) = false; + let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); + slot.dict = None; + slot.in_memory = false; } /// The `.dic` file of the installed Czech dictionary, searching the dictionaries folder and @@ -69,7 +74,7 @@ pub fn reload_dictionary() { /// none. A dictionary given with [`install_dictionary`] has no file and isn't reported, even if a /// matching file also happens to exist. pub fn installed_path() -> Option { - if *DICT_IN_MEMORY.read().unwrap_or_else(|e| e.into_inner()) { + if DICT.read().unwrap_or_else(|e| e.into_inner()).in_memory { return None; } crate::hunspell::find(NAMES).map(|(_, dic)| dic) diff --git a/crates/proof/src/ru/mod.rs b/crates/proof/src/ru/mod.rs index caba4f8aa..4b1ffe093 100644 --- a/crates/proof/src/ru/mod.rs +++ b/crates/proof/src/ru/mod.rs @@ -34,13 +34,16 @@ use crate::affix::AffixDict; pub use suggest::suggest; -/// The Russian dictionary: `None` until first use, then the installed one (or an empty one). -static DICT: RwLock>> = RwLock::new(None); +/// The Russian dictionary and whether it came from [`install_dictionary`] rather than a file on +/// disk (it then has no file of its own to report from [`installed_path`], even if a matching +/// file also happens to exist): one lock, so a reload and an install can't interleave their two +/// fields into an inconsistent combination. +static DICT: RwLock = RwLock::new(DictState { dict: None, in_memory: false }); -/// Whether `DICT` holds a dictionary given with [`install_dictionary`] rather than one found on -/// disk: it has no file of its own to report from [`installed_path`], even if a matching file -/// happens to exist too. -static DICT_IN_MEMORY: RwLock = RwLock::new(false); +struct DictState { + dict: Option>, + in_memory: bool, +} /// File names of a Russian Hunspell dictionary. const NAMES: &[&str] = &["ru_RU", "ru-RU", "ru"]; @@ -49,13 +52,13 @@ const NAMES: &[&str] = &["ru_RU", "ru-RU", "ru"]; /// [`install_dictionary`], or an empty one when none is available (Russian spelling is off then; /// see [`has_dictionary`]). pub fn dictionary() -> Arc { - if let Some(d) = DICT.read().unwrap_or_else(|e| e.into_inner()).as_ref() { + if let Some(d) = DICT.read().unwrap_or_else(|e| e.into_inner()).dict.as_ref() { return d.clone(); } let loaded = Arc::new(load_installed()); let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); // Another thread (or an install) may have filled it meanwhile: that one wins. - slot.get_or_insert(loaded).clone() + slot.dict.get_or_insert(loaded).clone() } fn load_installed() -> AffixDict { @@ -73,16 +76,18 @@ fn load_installed() -> AffixDict { /// for example one the user picked. On error the current dictionary stays. pub fn install_dictionary(aff: &str, dic: &str) -> Result<(), String> { let d = AffixDict::from_hunspell(aff, dic, &[])?; - *DICT.write().unwrap_or_else(|e| e.into_inner()) = Some(Arc::new(d)); - *DICT_IN_MEMORY.write().unwrap_or_else(|e| e.into_inner()) = true; + let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); + slot.dict = Some(Arc::new(d)); + slot.in_memory = true; Ok(()) } /// Forget the loaded dictionary, so the next check looks for an installed one again (after /// [`crate::hunspell::set_dictionary_dir`] or a dictionary was installed). pub fn reload_dictionary() { - *DICT.write().unwrap_or_else(|e| e.into_inner()) = None; - *DICT_IN_MEMORY.write().unwrap_or_else(|e| e.into_inner()) = false; + let mut slot = DICT.write().unwrap_or_else(|e| e.into_inner()); + slot.dict = None; + slot.in_memory = false; } /// The `.dic` file of the installed Russian dictionary, searching the dictionaries folder and @@ -90,7 +95,7 @@ pub fn reload_dictionary() { /// none. A dictionary given with [`install_dictionary`] has no file and isn't reported, even if a /// matching file also happens to exist. pub fn installed_path() -> Option { - if *DICT_IN_MEMORY.read().unwrap_or_else(|e| e.into_inner()) { + if DICT.read().unwrap_or_else(|e| e.into_inner()).in_memory { return None; } crate::hunspell::find(NAMES).map(|(_, dic)| dic)