diff --git a/Cargo.lock b/Cargo.lock index c2101d6..ffb1099 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -264,6 +264,16 @@ dependencies = [ "num-traits", ] +[[package]] +name = "fs4" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec6fcfb3c0c1d71612528825042261419d5dade9678c39a781e05b63677d9b32" +dependencies = [ + "rustix", + "windows-sys", +] + [[package]] name = "fs_extra" version = "1.3.0" @@ -605,13 +615,13 @@ dependencies = [ [[package]] name = "regex" -version = "1.10.5" +version = "1.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b91213439dad192326a0d7c6ee3955910425f441d7038e0d6933b0aec5c4517f" +checksum = "38200e5ee88914975b69f657f0801b6f6dccafd44fd9326302a4aaeecfacb1d8" dependencies = [ "aho-corasick", "memchr", - "regex-automata 0.4.7", + "regex-automata 0.4.8", "regex-syntax", ] @@ -623,9 +633,9 @@ checksum = "6c230d73fb8d8c1b9c0b3135c5142a8acee3a0558fb8db5cf1cb65f8d7862132" [[package]] name = "regex-automata" -version = "0.4.7" +version = "0.4.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "38caf58cc5ef2fed281f89292ef23f6365465ed9a41b7a7754eb4e26496c92df" +checksum = "368758f23274712b504848e9d5a6f010445cc8b87a7cdb4d7cbee666c1288da3" dependencies = [ "aho-corasick", "memchr", @@ -634,9 +644,9 @@ dependencies = [ [[package]] name = "regex-syntax" -version = "0.8.4" +version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7a66a03ae7c801facd77a29370b4faec201768915ac14a721ba36f20bc9c209b" +checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" [[package]] name = "relative-path" @@ -655,10 +665,12 @@ dependencies = [ "clap_complete", "clap_complete_nushell", "dunce", + "fs4", "fs_extra", "lazy_static", "predicates 3.1.0", "rand", + "regex", "rstest", "tempfile", "walkdir", diff --git a/Cargo.toml b/Cargo.toml index 111db23..f63bc7e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ clap = { version = "4.4", features = ["derive"] } clap_complete = "4.4" clap_complete_nushell = "4.4" dunce = "1.0.4" +fs4 = { version = "0.10.0", features = ["sync"] } fs_extra = "1.3" walkdir = "1" @@ -30,6 +31,7 @@ assert_cmd = "1.0" lazy_static = "1.4" predicates = "3.0" rand = "0.8" +regex = "1.11.0" rstest = "0.18" tempfile = "3" diff --git a/README.md b/README.md index 4f066d4..ba88d9e 100644 --- a/README.md +++ b/README.md @@ -22,9 +22,10 @@ This version, "rip2", is a fork-of-a-fork: - Added support for: Windows, NixOS - Cleanup: refactoring to modern rust, merging PRs from original repo - Testing: add full test suite and coverage monitoring - - Features: colorful output, datetime info in seance, + - Features: colorful output, datetime info in seance - Bug fixes: FIFO files, issue with seance - Shell completions for bash, elvish, fish, powershell, zsh, and nushell (via clap) + - Thread safety for deletion record ## ⚰️ Installation diff --git a/src/lib.rs b/src/lib.rs index a4dcf2b..fb014e1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,7 @@ pub mod record; pub mod util; use args::Args; -use record::{Record, RecordItem}; +use record::{Record, RecordItem, DEFAULT_FILE_LOCK}; const LINES_TO_INSPECT: usize = 6; const FILES_TO_INSPECT: usize = 6; @@ -42,7 +42,7 @@ pub fn run(cli: Args, mode: impl util::TestingMode, stream: &mut impl Write) -> } // Stores the deleted files - let record = Record::new(graveyard); + let record = Record::::new(graveyard); let cwd = &env::current_dir()?; // If the user wishes to restore everything @@ -119,10 +119,10 @@ pub fn run(cli: Args, mode: impl util::TestingMode, stream: &mut impl Write) -> Ok(()) } -fn bury_target( +fn bury_target( target: &PathBuf, graveyard: &PathBuf, - record: &Record, + record: &Record, cwd: &Path, inspect: bool, mode: &impl util::TestingMode, diff --git a/src/record.rs b/src/record.rs index df9288c..fb651b0 100644 --- a/src/record.rs +++ b/src/record.rs @@ -1,5 +1,6 @@ use chrono::Local; -use std::io::{BufRead, BufReader, Error, ErrorKind, Write}; +use fs4::fs_std::FileExt; +use std::io::{BufRead, BufReader, Error, ErrorKind, Read, Write}; use std::path::{Path, PathBuf}; use std::{fs, io}; @@ -29,13 +30,27 @@ impl RecordItem { } } +/// A record of file operations maintained in the graveyard directory +/// +/// # Type Parameters +/// +/// * `FILE_LOCK` - When `true`, exclusive file locks are acquired when opening +/// the record file for reading or writing. This prevents concurrent access from multiple +/// processes. When `false`, no file locking is performed - which is used for testing. #[derive(Debug)] -pub struct Record { +pub struct Record { path: PathBuf, } -impl Record { - pub fn new(graveyard: &Path) -> Record { +#[cfg(not(target_os = "windows"))] +pub const DEFAULT_FILE_LOCK: bool = true; + +#[cfg(target_os = "windows")] +pub const DEFAULT_FILE_LOCK: bool = false; +// TODO: Investigate why this is needed. Does Windows not support file locks? + +impl Record { + pub fn new(graveyard: &Path) -> Record { let path = graveyard.join(RECORD); // Create the record file if it doesn't exist if !path.exists() { @@ -46,6 +61,9 @@ impl Record { .write(true) .open(&path) .expect("Failed to open record file"); + if FILE_LOCK { + record_file.lock_exclusive().unwrap(); + } record_file .write_all(b"Time\tOriginal\tDestination\n") .expect("Failed to write header to record file"); @@ -54,8 +72,12 @@ impl Record { } pub fn open(&self) -> Result { - fs::File::open(&self.path) - .map_err(|_| Error::new(ErrorKind::NotFound, "Failed to read record!")) + let file = fs::File::open(&self.path) + .map_err(|_| Error::new(ErrorKind::NotFound, "Failed to read record!"))?; + if FILE_LOCK { + file.lock_exclusive().unwrap(); + } + Ok(file) } /// Return the path in the graveyard of the last file to be buried. @@ -64,10 +86,8 @@ impl Record { pub fn get_last_bury(&self) -> Result { // record: impl AsRef let record_file = self.open()?; - let contents = { - let path_f = PathBuf::from(&self.path); - fs::read_to_string(path_f)? - }; + let mut contents = String::new(); + BufReader::new(&record_file).read_to_string(&mut contents)?; // This will be None if there is nothing, or Some // if there is items in the vector @@ -109,10 +129,18 @@ impl Record { .map_while(Result::ok) .filter(|line| !graves.iter().any(|y| *y == RecordItem::new(line).dest)) .collect(); - let mut mutable_record_file = fs::File::create(record_path)?; - writeln!(mutable_record_file, "{}", header)?; // Write the header back + // let mut new_record_file = fs::File::create(record_path)?; + let mut new_record_file = fs::OpenOptions::new() + .create(true) + .truncate(true) + .write(true) + .open(record_path)?; + if FILE_LOCK { + new_record_file.lock_exclusive().unwrap(); + } + writeln!(new_record_file, "{}", header)?; // Write the header back for line in lines_to_write { - writeln!(mutable_record_file, "{}", line)?; + writeln!(new_record_file, "{}", line)?; } Ok(()) } @@ -160,18 +188,28 @@ impl Record { pub fn write_log(&self, source: impl AsRef, dest: impl AsRef) -> io::Result<()> { let (source, dest) = (source.as_ref(), dest.as_ref()); - // Check if record exists. If not, create it and write the header. - // TODO: Is this actually necessary? - if !self.path.exists() { - let mut record_file = fs::OpenOptions::new() + let already_existed = self.path.exists(); + + // TODO: The tiny amount of time between the check and the open + // could allow for a race condition. But maybe I'm being overkill. + + let mut record_file = if already_existed { + fs::OpenOptions::new().append(true).open(&self.path)? + } else { + fs::OpenOptions::new() .create(true) .truncate(true) .write(true) - .open(&self.path)?; - writeln!(record_file, "Time\tOriginal\tDestination")?; + .open(&self.path)? + }; + + if FILE_LOCK { + record_file.lock_exclusive().unwrap(); } - let mut record_file = fs::OpenOptions::new().append(true).open(&self.path)?; + if !already_existed { + writeln!(record_file, "Time\tOriginal\tDestination")?; + } writeln!( record_file, @@ -190,3 +228,11 @@ impl Record { Ok(()) } } + +impl Clone for Record { + fn clone(&self) -> Self { + Record { + path: self.path.clone(), + } + } +} diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index a213dfa..7812228 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -11,7 +11,7 @@ use std::fs; use std::hash::{DefaultHasher, Hash, Hasher}; use std::io::{BufReader, ErrorKind, Read, Write}; use std::path::PathBuf; -use std::sync::{Mutex, MutexGuard}; +use std::sync::{Arc, Barrier, Mutex, MutexGuard}; use std::{env, ffi, iter}; use tempfile::{tempdir, TempDir}; use walkdir::WalkDir; @@ -638,7 +638,7 @@ fn issue_0018() { assert!(!record_contents.contains("gnu_meta.zip")); // And give this for the last bury - let record = record::Record::new(&test_env.graveyard); + let record = record::Record::<{ record::DEFAULT_FILE_LOCK }>::new(&test_env.graveyard); let last_bury = record.get_last_bury().unwrap(); assert!(last_bury.ends_with("uu_meta.zip")); } @@ -708,7 +708,7 @@ fn read_empty_record() { let test_env = TestEnv::new(); let cwd = env::current_dir().unwrap(); fs::create_dir(&test_env.graveyard).unwrap(); - let record = record::Record::new(&test_env.graveyard); + let record = record::Record::<{ record::DEFAULT_FILE_LOCK }>::new(&test_env.graveyard); let gravepath = &util::join_absolute(&test_env.graveyard, dunce::canonicalize(cwd).unwrap()); let result = record.seance(gravepath); assert!(result.is_ok()); @@ -909,8 +909,8 @@ fn test_bury_unbury_bury_unbury() { // Get the record file's contents: let record_path = test_env.graveyard.join(record::RECORD); - assert!(record_path.exists()); - let record_contents = fs::read_to_string(&record_path).unwrap(); + assert!(record_path.clone().exists()); + let record_contents = fs::read_to_string(record_path.clone()).unwrap(); println!("Initial record contents:\n{}", record_contents); assert!(record_contents.contains(&normalized_test_data_path.display().to_string())); @@ -934,8 +934,8 @@ fn test_bury_unbury_bury_unbury() { assert_eq!(restored_data, test_data.data); // Get the new record file's contents: - assert!(record_path.exists()); - let record_contents = fs::read_to_string(&record_path).unwrap(); + assert!(record_path.clone().exists()); + let record_contents = fs::read_to_string(record_path.clone()).unwrap(); println!("After first unbury, record contents:\n{}", record_contents); // The record should still have the header: @@ -988,3 +988,92 @@ fn test_bury_unbury_bury_unbury() { let restored_data = fs::read_to_string(&test_data.path).unwrap(); assert_eq!(restored_data, test_data.data); } + +/// Test concurrent writes to the pre-existing record file +#[cfg(not(target_os = "windows"))] +#[rstest] +fn test_concurrent_writes(#[values(true, false)] file_lock: bool) { + if file_lock { + _test_concurrent_writes::(); + } else { + match std::thread::available_parallelism() { + Ok(num_threads) if num_threads.get() > 1 => { + _test_concurrent_writes::(); + } + _ => { + // If we don't have multiple threads, skip this test + println!( + "Warning: skipping test_concurrent_writes because we don't have multiple threads" + ); + } + } + } +} +fn _test_concurrent_writes() { + let _env_lock = aquire_lock(); + let test_env = TestEnv::new(); + fs::create_dir(&test_env.graveyard).unwrap(); + let record = record::Record::::new(&test_env.graveyard); + let record_path = test_env.graveyard.join(record::RECORD); + + // Create two threads that will write to the record simultaneously + let barrier = Arc::new(Barrier::new(2)); + + let barrier_from_1 = barrier.clone(); + let record_from_1 = record.clone(); + let handle1 = std::thread::spawn(move || { + barrier_from_1.wait(); + for i in 0..1000 { + record_from_1 + .write_log(format!("src_path_{}", i), format!("dest_path_{}", i)) + .unwrap(); + } + }); + + let barrier_from_2 = barrier.clone(); + let record_from_2 = record.clone(); + let handle2 = std::thread::spawn(move || { + barrier_from_2.wait(); + for i in 1000..2000 { + record_from_2 + .write_log(format!("src_path_{}", i), format!("dest_path_{}", i)) + .unwrap(); + } + }); + + // Wait for both threads to complete + handle1.join().unwrap(); + handle2.join().unwrap(); + + let record_contents = fs::read_to_string(record_path.clone()).unwrap(); + + // The file should be perfectly formatted if `with_locking` is true, + // but corrupted if it is not + if FILE_LOCK { + assert!(record_contents.contains("Time")); + assert!(record_contents.contains("Original")); + assert!(record_contents.contains("Destination")); + } + + let lines: Vec<&str> = record_contents.lines().collect(); + + if FILE_LOCK { + assert_eq!(lines.len(), 2001); + } + + // Check each of the 2000 lines for corruption + let re = regex::Regex::new( + r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(Z|[+-]\d{2}:\d{2})\t.+\t.+$", + ) + .unwrap(); + let corrupted_lines = lines + .iter() + .skip(1) + .filter(|line| !re.is_match(line)) + .count(); + if FILE_LOCK { + assert_eq!(corrupted_lines, 0); + } else { + assert!(corrupted_lines > 0); + } +}