Log when FileLock::drop fails to unlock file (#2064)

* Add optional dependency tracing to fs-lock

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Add optional logging to FileLock::drop

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Enable fs-lock/tracing in binstalk-manifest

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Add FileLock::set_file_path

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Update create_if_not_exist to return FileLock on successs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Update create_if_not_exist usage in mod binstall_crates_v1

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Simplify create_if_not_exist

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Refactor mod crates_manifests to use create_if_not_exist

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Set file path for FileLock

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Set file path for file lock in mod cargo_config

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fs-lock impl

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Import Path in fs-lock lib.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fs-lock lib.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fs-lock lib.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix typo in crates_manifests.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix calling create_if_not_exist in crates_manifests.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fox fmt crates_manifests.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fmt in lib.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

---------

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

crates/binstalk-manifests/Cargo.toml

@@ -13,7 +13,7 @@ license = "Apache-2.0 OR MIT"
 beef = { version = "0.5.2", features = ["impl_serde"] }
 binstalk-types = { version = "0.9.3", path = "../binstalk-types" }
 compact_str = { version = "0.8.0", features = ["serde"] }
-fs-lock = { version = "0.1.7", path = "../fs-lock" }
+fs-lock = { version = "0.1.7", path = "../fs-lock", features = ["tracing"] }
 home = "0.5.9"
 miette = "7.0.0"
 semver = { version = "1.0.17", features = ["serde"] }

crates/binstalk-manifests/src/binstall_crates_v1.rs

@@ -43,7 +43,8 @@ where
     Iter: IntoIterator<Item = T>,
     Data: From<T>,
 {
-    let mut file = FileLock::new_exclusive(create_if_not_exist(path.as_ref())?)?;
+    let path = path.as_ref();
+    let mut file = create_if_not_exist(path)?;
     // Move the cursor to EOF
     file.seek(io::SeekFrom::End(0))?;
 
@@ -166,7 +167,7 @@ impl Records {
 
     pub fn load_from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
         let mut this = Self {
-            file: FileLock::new_exclusive(create_if_not_exist(path.as_ref())?)?,
+            file: create_if_not_exist(path.as_ref())?,
             data: BTreeSet::default(),
         };
         this.load_impl()?;

crates/binstalk-manifests/src/cargo_config.rs

@@ -138,7 +138,7 @@ impl Config {
         fn inner(path: &Path) -> Result<Config, ConfigLoadError> {
             match File::open(path) {
                 Ok(file) => {
-                    let file = FileLock::new_shared(file)?;
+                    let file = FileLock::new_shared(file)?.set_file_path(path);
                     // Any regular file must have a parent dir
                     Config::load_from_reader(file, path.parent().unwrap())
                 }

crates/binstalk-manifests/src/cargo_crates_v1.rs

@@ -62,7 +62,8 @@ impl CratesToml<'_> {
     }
 
     pub fn load_from_path(path: impl AsRef<Path>) -> Result<Self, CratesTomlParseError> {
-        let file = FileLock::new_shared(File::open(path)?)?;
+        let path = path.as_ref();
+        let file = FileLock::new_shared(File::open(path)?)?.set_file_path(path);
         Self::load_from_reader(file)
     }
 
@@ -100,7 +101,8 @@ impl CratesToml<'_> {
     }
 
     pub fn write_to_path(&self, path: impl AsRef<Path>) -> Result<(), CratesTomlParseError> {
-        let mut file = FileLock::new_exclusive(File::create(path)?)?;
+        let path = path.as_ref();
+        let mut file = FileLock::new_exclusive(File::create(path)?)?.set_file_path(path);
         self.write_to_file(&mut file)
     }
 
@@ -142,7 +144,7 @@ impl CratesToml<'_> {
     where
         Iter: IntoIterator<Item = &'a CrateInfo>,
     {
-        let mut file = FileLock::new_exclusive(create_if_not_exist(path.as_ref())?)?;
+        let mut file = create_if_not_exist(path.as_ref())?;
         Self::append_to_file(&mut file, iter)
     }
 

crates/binstalk-manifests/src/crates_manifests.rs

@@ -13,6 +13,7 @@ use crate::{
     binstall_crates_v1::{Error as BinstallCratesV1Error, Records as BinstallCratesV1Records},
     cargo_crates_v1::{CratesToml, CratesTomlParseError},
     crate_info::CrateInfo,
+    helpers::create_if_not_exist,
     CompactString, Version,
 };
 
@@ -47,13 +48,7 @@ impl Manifests {
         // Read cargo_install_v1_metadata
         let manifest_path = cargo_roots.join(".crates.toml");
 
-        let cargo_crates_v1 = fs::File::options()
+        let cargo_crates_v1 = create_if_not_exist(&manifest_path)?;
-            .read(true)
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .open(manifest_path)
-            .and_then(FileLock::new_exclusive)?;
 
         Ok(Self {
             binstall,

crates/binstalk-manifests/src/helpers.rs

@@ -1,13 +1,15 @@
 use std::{fs, io, path::Path};
 
-/// Returned file is readable and writable.
+use fs_lock::FileLock;
-pub(crate) fn create_if_not_exist(path: &Path) -> io::Result<fs::File> {
-    let mut options = fs::File::options();
-    options.read(true).write(true);
 
-    options
+/// Return exclusively locked file that is readable and writable.
-        .clone()
+pub(crate) fn create_if_not_exist(path: &Path) -> io::Result<FileLock> {
-        .create_new(true)
+    fs::File::options()
+        .read(true)
+        .write(true)
+        .create(true)
+        .truncate(false)
         .open(path)
-        .or_else(|_| options.open(path))
+        .and_then(FileLock::new_exclusive)
+        .map(|file_lock| file_lock.set_file_path(path))
 }

crates/fs-lock/Cargo.toml

@@ -11,3 +11,4 @@ license = "Apache-2.0 OR MIT"
 
 [dependencies]
 fs4 = "0.12.0"
+tracing = { version = "0.1", optional = true }

crates/fs-lock/src/lib.rs

@@ -6,22 +6,33 @@ use std::{
     fs::File,
     io::{self, IoSlice, IoSliceMut, SeekFrom},
     ops,
+    path::Path,
 };
 
 use fs4::fs_std::FileExt;
 
 /// A locked file.
 #[derive(Debug)]
-pub struct FileLock(File);
+pub struct FileLock(File, #[cfg(feature = "tracing")] Option<Box<Path>>);
 
 impl FileLock {
+    #[cfg(not(feature = "tracing"))]
+    fn new(file: File) -> Self {
+        Self(file)
+    }
+
+    #[cfg(feature = "tracing")]
+    fn new(file: File) -> Self {
+        Self(file, None)
+    }
+
     /// Take an exclusive lock on a [`File`].
     ///
     /// Note that this operation is blocking, and should not be called in async contexts.
     pub fn new_exclusive(file: File) -> io::Result<Self> {
         FileExt::lock_exclusive(&file)?;
 
-        Ok(Self(file))
+        Ok(Self::new(file))
     }
 
     /// Try to take an exclusive lock on a [`File`].
@@ -33,7 +44,7 @@ impl FileLock {
     /// Note that this operation is blocking, and should not be called in async contexts.
     pub fn new_try_exclusive(file: File) -> Result<Self, (File, Option<io::Error>)> {
         match FileExt::try_lock_exclusive(&file) {
-            Ok(()) => Ok(Self(file)),
+            Ok(()) => Ok(Self::new(file)),
             Err(e) if e.raw_os_error() == fs4::lock_contended_error().raw_os_error() => {
                 Err((file, None))
             }
@@ -47,7 +58,7 @@ impl FileLock {
     pub fn new_shared(file: File) -> io::Result<Self> {
         FileExt::lock_shared(&file)?;
 
-        Ok(Self(file))
+        Ok(Self::new(file))
     }
 
     /// Try to take a shared lock on a [`File`].
@@ -59,18 +70,47 @@ impl FileLock {
     /// Note that this operation is blocking, and should not be called in async contexts.
     pub fn new_try_shared(file: File) -> Result<Self, (File, Option<io::Error>)> {
         match FileExt::try_lock_shared(&file) {
-            Ok(()) => Ok(Self(file)),
+            Ok(()) => Ok(Self::new(file)),
             Err(e) if e.raw_os_error() == fs4::lock_contended_error().raw_os_error() => {
                 Err((file, None))
             }
             Err(e) => Err((file, Some(e))),
         }
     }
+
+    /// Set path to the file for logging on unlock error, if feature tracing is enabled
+    pub fn set_file_path(mut self, path: impl Into<Box<Path>>) -> Self {
+        #[cfg(feature = "tracing")]
+        {
+            self.1 = Some(path.into());
+        }
+        self
+    }
 }
 
 impl Drop for FileLock {
     fn drop(&mut self) {
-        let _ = FileExt::unlock(&self.0);
+        let _res = FileExt::unlock(&self.0);
+        #[cfg(feature = "tracing")]
+        if let Err(err) = _res {
+            use std::fmt;
+
+            struct OptionalPath<'a>(Option<&'a Path>);
+            impl fmt::Display for OptionalPath<'_> {
+                fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                    if let Some(path) = self.0 {
+                        fmt::Display::fmt(&path.display(), f)
+                    } else {
+                        Ok(())
+                    }
+                }
+            }
+
+            tracing::warn!(
+                "Failed to unlock file{}: {err}",
+                OptionalPath(self.1.as_deref()),
+            );
+        }
     }
 }