diff --git a/crates/binstalk-manifests/Cargo.toml b/crates/binstalk-manifests/Cargo.toml index 47fc7066..f42a69d1 100644 --- a/crates/binstalk-manifests/Cargo.toml +++ b/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"] } diff --git a/crates/binstalk-manifests/src/binstall_crates_v1.rs b/crates/binstalk-manifests/src/binstall_crates_v1.rs index e0234f77..e9282d61 100644 --- a/crates/binstalk-manifests/src/binstall_crates_v1.rs +++ b/crates/binstalk-manifests/src/binstall_crates_v1.rs @@ -43,7 +43,8 @@ where Iter: IntoIterator, Data: From, { - 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) -> Result { 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()?; diff --git a/crates/binstalk-manifests/src/cargo_config.rs b/crates/binstalk-manifests/src/cargo_config.rs index 6bc36d96..dc9985dc 100644 --- a/crates/binstalk-manifests/src/cargo_config.rs +++ b/crates/binstalk-manifests/src/cargo_config.rs @@ -138,7 +138,7 @@ impl Config { fn inner(path: &Path) -> Result { 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()) } diff --git a/crates/binstalk-manifests/src/cargo_crates_v1.rs b/crates/binstalk-manifests/src/cargo_crates_v1.rs index a4821285..3cf0794e 100644 --- a/crates/binstalk-manifests/src/cargo_crates_v1.rs +++ b/crates/binstalk-manifests/src/cargo_crates_v1.rs @@ -62,7 +62,8 @@ impl CratesToml<'_> { } pub fn load_from_path(path: impl AsRef) -> Result { - 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) -> 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, { - 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) } diff --git a/crates/binstalk-manifests/src/crates_manifests.rs b/crates/binstalk-manifests/src/crates_manifests.rs index 265e1e0d..3208b5e5 100644 --- a/crates/binstalk-manifests/src/crates_manifests.rs +++ b/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() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(manifest_path) - .and_then(FileLock::new_exclusive)?; + let cargo_crates_v1 = create_if_not_exist(&manifest_path)?; Ok(Self { binstall, diff --git a/crates/binstalk-manifests/src/helpers.rs b/crates/binstalk-manifests/src/helpers.rs index 029564c9..45d55fb8 100644 --- a/crates/binstalk-manifests/src/helpers.rs +++ b/crates/binstalk-manifests/src/helpers.rs @@ -1,13 +1,15 @@ use std::{fs, io, path::Path}; -/// Returned file is readable and writable. -pub(crate) fn create_if_not_exist(path: &Path) -> io::Result { - let mut options = fs::File::options(); - options.read(true).write(true); +use fs_lock::FileLock; - options - .clone() - .create_new(true) +/// Return exclusively locked file that is readable and writable. +pub(crate) fn create_if_not_exist(path: &Path) -> io::Result { + 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)) } diff --git a/crates/fs-lock/Cargo.toml b/crates/fs-lock/Cargo.toml index 04d36516..6306e32a 100644 --- a/crates/fs-lock/Cargo.toml +++ b/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 } diff --git a/crates/fs-lock/src/lib.rs b/crates/fs-lock/src/lib.rs index 0bf20a96..abf60873 100644 --- a/crates/fs-lock/src/lib.rs +++ b/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>); 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 { 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)> { 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 { 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)> { 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>) -> 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()), + ); + } } }