diff --git a/crates/storage/db/src/lockfile.rs b/crates/storage/db/src/lockfile.rs index 6b2aac3df8..2d7704506b 100644 --- a/crates/storage/db/src/lockfile.rs +++ b/crates/storage/db/src/lockfile.rs @@ -9,31 +9,34 @@ use std::{ }; use sysinfo::System; -/// A file lock for a storage directory to ensure exclusive read-write access. +/// File lock name. +const LOCKFILE_NAME: &str = "lock"; + +/// A file lock for a storage directory to ensure exclusive read-write access across different +/// processes. /// /// This lock stores the PID of the process holding it and is released (deleted) on a graceful /// shutdown. On resuming from a crash, the stored PID helps verify that no other process holds the /// lock. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct StorageLock(Arc); impl StorageLock { /// Tries to acquire a write lock on the target directory, returning [`StorageLockError`] if /// unsuccessful. + /// + /// Note: In-process exclusivity is not on scope. If called from the same process (or another + /// with the same PID), it will succeed. pub fn try_acquire(path: &Path) -> Result { - let path = path.join("lock"); - let lock = match parse_lock_file_pid(&path)? { - Some(pid) => { - if System::new_all().process(pid.into()).is_some() { - return Err(StorageLockError::Taken(pid)) - } else { - // If PID is no longer active, take hold of the lock. - StorageLockInner::new(path) - } + let path = path.join(LOCKFILE_NAME); + + if let Some(pid) = parse_lock_file_pid(&path)? { + if pid != (process::id() as usize) && System::new_all().process(pid.into()).is_some() { + return Err(StorageLockError::Taken(pid)) } - None => StorageLockInner::new(path), - }; - Ok(Self(Arc::new(lock?))) + } + + Ok(Self(Arc::new(StorageLockInner::new(path)?))) } } @@ -50,7 +53,7 @@ impl Drop for StorageLock { } } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] struct StorageLockInner { file_path: PathBuf, } @@ -77,3 +80,44 @@ fn parse_lock_file_pid(path: &Path) -> Result, StorageLockError> { } Ok(None) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_lock() { + let temp_dir = tempfile::tempdir().unwrap(); + + let lock = StorageLock::try_acquire(temp_dir.path()).unwrap(); + + // Same process can re-acquire the lock + assert_eq!(Ok(lock.clone()), StorageLock::try_acquire(temp_dir.path())); + + // A lock of a non existent PID can be acquired. + let lock_file = temp_dir.path().join(LOCKFILE_NAME); + let mut fake_pid = 1337; + let system = System::new_all(); + while system.process(fake_pid.into()).is_some() { + fake_pid += 1; + } + reth_fs_util::write(&lock_file, format!("{}", fake_pid)).unwrap(); + assert_eq!(Ok(lock), StorageLock::try_acquire(temp_dir.path())); + + // A lock of a different but existing PID cannot be acquired. + reth_fs_util::write(&lock_file, "1").unwrap(); + assert_eq!(Err(StorageLockError::Taken(1)), StorageLock::try_acquire(temp_dir.path())); + } + + #[test] + fn test_drop_lock() { + let temp_dir = tempfile::tempdir().unwrap(); + let lock_file = temp_dir.path().join(LOCKFILE_NAME); + + let lock = StorageLock::try_acquire(temp_dir.path()).unwrap(); + + drop(lock); + + assert!(!lock_file.exists()); + } +} diff --git a/crates/storage/errors/src/lockfile.rs b/crates/storage/errors/src/lockfile.rs index e5d4181a14..674485457a 100644 --- a/crates/storage/errors/src/lockfile.rs +++ b/crates/storage/errors/src/lockfile.rs @@ -5,7 +5,7 @@ use thiserror::Error; /// Storage lock error. pub enum StorageLockError { /// Write lock taken - #[error("storage directory is currently in use as read-write by another process: {0}")] + #[error("storage directory is currently in use as read-write by another process: PID {0}")] Taken(usize), /// Indicates other unspecified errors. #[error("{0}")]