Reduce scope of file mutex

Author: rossmacarthur

Cargo.lock

@@ -33,7 +33,7 @@ casual = "0.2.0"
 clap_complete = "4.4.10"
 constcat = "0.6.0"
 curl = "0.4.46"
-fmutex = "0.1.0"
+fmutex = "0.3.0"
 git2 = "0.20.0"
 globwalk = "0.9.1"
 home = "0.5.9"

Cargo.toml

@@ -1,4 +1,4 @@
-<!-- Generated by cargo-onedoc. DO NOT EDIT. -->
+<!-- Generated by cargo-onedoc v0.2.2. DO NOT EDIT. -->
 
 # sheldon
 

README.md

@@ -1,5 +1,11 @@
 # 📝 Release notes
 
+## 0.8.2
+
+*Unreleased*
+
+- [Reduce scope of file mutex][todo]. This fixes #196.
+
 ## 0.8.1
 
 *February 8th, 2025*

RELEASES.md

@@ -1,5 +1,3 @@
-<!-- Generated by cargo-onedoc. DO NOT EDIT. -->
-
 # sheldon
 
 *Fast, configurable, shell plugin manager*

docs/README_TEMPLATE.md

@@ -226,9 +226,6 @@ impl clap::ValueEnum for Shell {
 fn key_value_parser(s: &str) -> Result<(String, String), String> {
     match s.split_once('=') {
         Some((k, v)) => Ok((k.to_string(), v.to_string())),
-        _ => Err(format!(
-            "{} isn't a valid key-value pair separated with =",
-            s
-        )),
+        _ => Err(format!("{s} isn't a valid key-value pair separated with =")),
     }
 }

src/cli/raw.rs

@@ -40,15 +40,6 @@ fn main() {
 
 /// The main entry point to execute the application.
 pub fn run_command(ctx: &Context, command: Command) -> Result<()> {
-    // We always try to acquire the mutex but it is only strictly necessary for
-    // the lock and source commands.
-    let _guard = match acquire_mutex(ctx, ctx.config_dir()) {
-        Ok(g) => Some(g),
-        Err(_) if !matches!(command, Command::Lock | Command::Source) => None,
-        Err(err) => {
-            return Err(err).context("failed to acquire lock on config directory");
-        }
-    };
     let mut warnings = Vec::new();
     let result = match command {
         Command::Init { shell } => init(ctx, shell),
@@ -64,27 +55,11 @@ pub fn run_command(ctx: &Context, command: Command) -> Result<()> {
     result
 }
 
-fn acquire_mutex(ctx: &Context, path: &Path) -> Result<fmutex::Guard> {
-    match fmutex::try_lock(path).with_context(|| format!("failed to open `{}`", path.display()))? {
-        Some(g) => Ok(g),
-        None => {
-            ctx.log_warning(
-                "Blocking",
-                &format!(
-                    "waiting for file lock on {}",
-                    ctx.replace_home(path).display()
-                ),
-            );
-            fmutex::lock(path)
-                .with_context(|| format!("failed to acquire file lock `{}`", path.display()))
-        }
-    }
-}
-
 /// Executes the `init` subcommand.
 ///
 /// Initialize a new config file.
 fn init(ctx: &Context, shell: Option<Shell>) -> Result<()> {
+    let _guard = access(ctx, Access::W);
     let path = ctx.config_file();
     match path
         .metadata()
@@ -105,6 +80,7 @@ fn init(ctx: &Context, shell: Option<Shell>) -> Result<()> {
 ///
 /// Add a new plugin to the config file.
 fn add(ctx: &Context, name: String, plugin: &EditPlugin) -> Result<()> {
+    let _guard = access(ctx, Access::W);
     let path = ctx.config_file();
     let mut config = match EditConfig::from_path(path) {
         Ok(config) => {
@@ -124,6 +100,7 @@ fn add(ctx: &Context, name: String, plugin: &EditPlugin) -> Result<()> {
 ///
 /// Open up the config file in the default editor.
 fn edit(ctx: &Context) -> Result<()> {
+    let _guard = access(ctx, Access::W);
     let path = ctx.config_file();
     let original_contents = match fs::read_to_string(path)
         .with_context(|| format!("failed to read from `{}`", path.display()))
@@ -152,6 +129,7 @@ fn edit(ctx: &Context) -> Result<()> {
 ///
 /// Remove a plugin from the config file.
 fn remove(ctx: &Context, name: String) -> Result<()> {
+    let _guard = access(ctx, Access::W);
     let path = ctx.config_file();
     let mut config = EditConfig::from_path(path)?;
     ctx.log_header("Loaded", path);
@@ -191,6 +169,8 @@ fn init_config(ctx: &Context, shell: Option<Shell>, path: &Path, err: Error) ->
 ///
 /// Install the plugins sources and generate the lock file.
 fn lock(ctx: &Context, warnings: &mut Vec<Error>) -> Result<()> {
+    let _guard = access(ctx, Access::W);
+
     let mut locked = locked(ctx, warnings)?;
 
     if let Some(last) = locked.errors.pop() {
@@ -216,36 +196,49 @@ fn source(ctx: &Context, warnings: &mut Vec<Error>) -> Result<()> {
     let mut to_path = true;
 
     let locked_config = if ctx.lock_mode.is_some() || newer_than(config_path, lock_path) {
+        let _g = access(ctx, Access::W)?;
         locked(ctx, warnings)?
     } else {
-        match lock::from_path(lock_path) {
+        let cfg = {
+            let _g = access(ctx, Access::R)?;
+            lock::from_path(lock_path)
+        };
+        match cfg {
             Ok(locked_config) => {
                 if locked_config.verify(ctx) {
                     to_path = false;
                     ctx.log_verbose_header("Unlocked", lock_path);
                     locked_config
                 } else {
+                    let _g = access(ctx, Access::W)?;
                     locked(ctx, warnings)?
                 }
             }
-            Err(_) => locked(ctx, warnings)?,
+            Err(_) => {
+                let _g = access(ctx, Access::W)?;
+                locked(ctx, warnings)?
+            }
         }
     };
 
-    let script = locked_config
-        .script(ctx)
-        .context("failed to render source")?;
+    let script = {
+        let _g = access(ctx, Access::R)?;
+        let script = locked_config
+            .script(ctx)
+            .context("failed to render source")?;
 
-    if to_path && locked_config.errors.is_empty() {
-        locked_config
-            .to_path(lock_path)
-            .context("failed to write lock file")?;
-        ctx.log_header("Locked", lock_path);
-    } else {
-        for err in &locked_config.errors {
-            ctx.log_error(err);
+        if to_path && locked_config.errors.is_empty() {
+            locked_config
+                .to_path(lock_path)
+                .context("failed to write lock file")?;
+            ctx.log_header("Locked", lock_path);
+        } else {
+            for err in &locked_config.errors {
+                ctx.log_error(err);
+            }
         }
-    }
+        script
+    };
 
     print!("{script}");
     Ok(())
@@ -269,3 +262,56 @@ fn locked(ctx: &Context, warnings: &mut Vec<Error>) -> Result<LockedConfig> {
     config::clean(ctx, warnings, &config)?;
     lock::config(ctx, config)
 }
+
+#[derive(Debug, Clone, Copy)]
+enum Access {
+    R,
+    W,
+}
+
+fn access(ctx: &Context, mode: Access) -> Result<fmutex::Guard<'static>> {
+    match mode {
+        Access::R => lock_read(ctx).context("failed to acquire exclusive lock on config directory"),
+        Access::W => lock_write(ctx).context("failed to acquire shared lock on config directory"),
+    }
+}
+
+fn lock_write(ctx: &Context) -> Result<fmutex::Guard<'static>> {
+    let path = ctx.config_dir();
+    match fmutex::try_lock_exclusive_path(path)
+        .with_context(|| format!("failed to open `{}`", path.display()))?
+    {
+        Some(g) => Ok(g),
+        None => {
+            ctx.log_warning(
+                "Blocking",
+                &format!(
+                    "waiting for file lock on {}",
+                    ctx.replace_home(path).display()
+                ),
+            );
+            fmutex::lock_exclusive_path(path)
+                .with_context(|| format!("failed to acquire file lock `{}`", path.display()))
+        }
+    }
+}
+
+fn lock_read(ctx: &Context) -> Result<fmutex::Guard<'static>> {
+    let path = ctx.config_dir();
+    match fmutex::try_lock_shared_path(path)
+        .with_context(|| format!("failed to open `{}`", path.display()))?
+    {
+        Some(g) => Ok(g),
+        None => {
+            ctx.log_warning(
+                "Blocking",
+                &format!(
+                    "waiting for file lock on {}",
+                    ctx.replace_home(path).display()
+                ),
+            );
+            fmutex::lock_shared_path(path)
+                .with_context(|| format!("failed to acquire file lock `{}`", path.display()))
+        }
+    }
+}