Draft: Call scan_suffixes when necessary during rotation, if files have been moved/created/deleted outside of this library#18
Conversation
kstrafe
left a comment
kstrafe
left a comment
There was a problem hiding this comment.
Not sure I fully understand some parts of it, specifically why it is important to not overwrite the file which we're moving the temporary file to.
| let actions = [Create(4), Remove(1), Remove(2), Remove(3)]; | ||
|
|
||
| for action in actions { | ||
| println!("{:?}", action); |
| let mut log = FileRotate::new( | ||
| &log_path, | ||
| AppendCount::new(5), | ||
| ContentLimit::Bytes(1), | ||
| Compression::None, | ||
| #[cfg(unix)] | ||
| None, | ||
| ); |
There was a problem hiding this comment.
This test looks like 4 different tests to me since we recreate a FileRotate. Could we instead actually make 4 tests (and perhaps just put common code in a function)? This test is a bit hard to read right now.
| assert_eq!(expected_set, log.suffixes); | ||
| } | ||
| } | ||
| println!("OK"); |
| if new_path.exists() { | ||
| return Err(MoveFileError::SuffixScanNeeded); | ||
| } |
There was a problem hiding this comment.
This seems very racy to me. If the new_path.exists() == false, a user can still modify the filesystem before we reach fs::rename on line 586. What is the intent of the code here? Won't rescanning suffixes just overwrite said file anyway?
There was a problem hiding this comment.
You're right about the race condition.
Rescanning suffixes does not overwrite anything in the fs, it just recreates self.suffixes which is only an internal cache of which log files are on disk. For correct functioning, we rely on the assumption that self.suffixes is correct. If it is not correct (user/system has done file ops), and if not for the asserts which up until now have served as the sole line of defense, we risk overwriting previous log files. Because fn move_file_with_suffix will only move away the destination file if it thinks it exists (look into the recursive nature of the function - it calls itself before fs::rename for this purpose).
2 participants
make_file_with_suffixfunction now returns a specific error if it fails due to failure of the assumption that files are not moved around / created / deleted by any other force than this library.fn rotateif it receives said error and callsself.scan_suffixes()before it tries againI currently test with
AppendCount.I want a test for AppendTimestamp too, not just AppendCount. Because the implementation feels a bit finicky.
Closes #17