diff --git a/src/lib.rs b/src/lib.rs index bb88bdf..73e0e36 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -394,6 +394,25 @@ pub struct FileRotate { mode: Option, } +/// Error for the function `move_file_with_suffix` - in order to +#[derive(Debug)] +enum MoveFileError { + Io(io::Error), + /// Safeguard for the assumption that the rotated files will not be moved/deleted + /// by an external process / user: + /// If the condition is true, that has likely happened and thus we need to call + /// `scan_suffixes` before trying again. + /// + /// If we were to assume said assumption, we would only call `scan_suffixes` + /// in FileRotate::new(). + SuffixScanNeeded, +} +impl From for MoveFileError { + fn from(e: io::Error) -> Self { + Self::Io(e) + } +} + impl FileRotate { /// Create a new [FileRotate]. /// @@ -507,17 +526,23 @@ impl FileRotate { /// Recursive function that keeps moving files if there's any file name collision. /// If `suffix` is `None`, it moves from basepath to next suffix given by the SuffixScheme - /// Assumption: Any collision in file name is due to an old log file. /// /// Returns the suffix of the new file (the last suffix after possible cascade of renames). fn move_file_with_suffix( &mut self, old_suffix_info: Option>, - ) -> io::Result> { + ) -> Result, MoveFileError> { + let old_path = match old_suffix_info { + Some(ref suffix) => suffix.to_path(&self.basepath), + None => self.basepath.clone(), + }; + if !old_path.exists() { + return Err(MoveFileError::SuffixScanNeeded); + } + // NOTE: this newest_suffix is there only because AppendTimestamp specifically needs // it. Otherwise it might not be necessary to provide this to `rotate_file`. We could also // have passed the internal BTreeMap itself, but it would require to make SuffixInfo `pub`. - let newest_suffix = self.suffixes.iter().next().map(|info| &info.suffix); let new_suffix = self.suffix_scheme.rotate_file( @@ -551,10 +576,9 @@ impl FileRotate { new_suffix_info }; - let old_path = match old_suffix_info { - Some(suffix) => suffix.to_path(&self.basepath), - None => self.basepath.clone(), - }; + if new_path.exists() { + return Err(MoveFileError::SuffixScanNeeded); + } // Do the move assert!(old_path.exists()); @@ -569,8 +593,20 @@ impl FileRotate { let _ = self.file.take(); - // This function will always create a new file. Returns suffix of that file - let new_suffix_info = self.move_file_with_suffix(None)?; + // `move_file_with_suffix` will always (on success) create a new file and return the suffix + // of that file. + // We need a loop here in case it fails for the specific reason that `self.suffixes` is not + // up-to-date, and we need another scan before moving on. + // The only reason for such a discrepancy is external interventions in the filesystem. + let new_suffix_info = loop { + match self.move_file_with_suffix(None) { + Ok(x) => break x, + Err(MoveFileError::SuffixScanNeeded) => { + self.scan_suffixes(); + } + Err(MoveFileError::Io(e)) => return Err(e), + } + }; self.suffixes.insert(new_suffix_info); self.open_file(); diff --git a/src/tests.rs b/src/tests.rs index 540b99a..959289b 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,4 +1,5 @@ use super::{suffix::*, *}; +use std::iter::FromIterator; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use tempdir::TempDir; @@ -371,6 +372,84 @@ fn unix_file_permissions() { } } +#[test] +fn user_moves_log_files() { + // FileRotate will be able to recover if its memory of what files exist (`self.suffixes`) turns + // out to not be consistent with the files that are actually on disk... User or external + // process has changed the file listing. + + let initial_suffixes = [1, 2, 3]; + + #[derive(Debug)] + enum Action { + Create(usize), + Remove(usize), + } + use Action::*; + + let actions = [Create(4), Remove(1), Remove(2), Remove(3)]; + + for action in actions { + println!("{:?}", action); + let tmp_dir = TempDir::new("file-rotate-test").unwrap(); + let dir = tmp_dir.path(); + let log_path = dir.join("log"); + for suffix in initial_suffixes { + File::create(dir.join(format!("log.{}", suffix))).unwrap(); + } + + let mut log = FileRotate::new( + &log_path, + AppendCount::new(5), + ContentLimit::Bytes(1), + Compression::None, + #[cfg(unix)] + None, + ); + + match action { + Create(suffix) => { + let name = format!("log.{}", suffix); + File::create(dir.join(name)).unwrap(); + } + Remove(suffix) => { + let name = format!("log.{}", suffix); + std::fs::remove_file(dir.join(name)).unwrap(); + } + } + + write!(log, "12").unwrap(); + + let mut expected_set = BTreeSet::from_iter(initial_suffixes.iter().map(|x| SuffixInfo { + suffix: *x, + compressed: false, + })); + match action { + Create(4) => { + // The one that was created + expected_set.insert(SuffixInfo { + suffix: 4, + compressed: false, + }); + // An additional one due to rotation + expected_set.insert(SuffixInfo { + suffix: 5, + compressed: false, + }); + assert_eq!(log.suffixes, expected_set); + } + // There is only one Create(_) action that is reasonable to test + Create(_) => unreachable!(), + Remove(_) => { + // No matter which file we remove, we would expect rotation to create one + // additional file such that we end up with the same files as we started. + assert_eq!(expected_set, log.suffixes); + } + } + println!("OK"); + } +} + #[quickcheck_macros::quickcheck] fn arbitrary_lines(count: usize) { let tmp_dir = TempDir::new("file-rotate-test").unwrap();