path check before log [CPP-851] #885
Conversation
adrian-kong
requested review from
a team,
john-michaelburke,
silverjam and
ziwift
| /// # Parameters: | ||
| /// - `frame`: The raw incoming data frame | ||
| /// - `msg`: Parsed message if present | ||
| pub fn serialize(&mut self, frame: &Frame, msg: Option<&Sbp>) { |
There was a problem hiding this comment.
probably better to just have one or the other parameter but since we want to keep a copy of the computed frame -> msg to prevent recalculation, just keep both params
There was a problem hiding this comment.
also NOTE: this isn't just serialize, it also updates the sbp formats which might be nice to seperate into a different method and have this only do serialization stuff
console_backend/src/output.rs
Outdated
| CompactFormatter, | ||
| ))) | ||
| } | ||
| if let Some(path) = &self.path { |
There was a problem hiding this comment.
not sure how costly this is checking everytime we serialize.
looks like TOCTOU 👀 but it should be ok
or alternatively use some sort of file notify.
There was a problem hiding this comment.
I think using something like notify or watchexec is a good idea but may require a bigger refactor than this bug necessitates. At the same time I don't think this check actually prevents all the bugs you are trying to prevent here, and it may create some new ones. You are just checking if the path exists, which could happen if someone deleted the file and then recreated it, this check would pass, but the write_all path would drop.
Another issue is that if you rename the file with mv the code before this change will keep writing to the renamed file, whereas this change will cause an unnecessary error imo.
Maybe these are tradeoffs that are ok to make, I don't have the context here and the jira tickets are minimal
There was a problem hiding this comment.
yea, this would address file deletion but file moves and instantly recreated would be kinda edge case (can fix by just ensuring metadata / size of file path before writing as well - not sure how much performance that is tho) 🤔
console_backend/src/output.rs
Outdated
| pub fn new(logger: SbpLogging, out: W, path: Option<PathBuf>) -> Self { | ||
| Self { logger, out, path } | ||
| } |
There was a problem hiding this comment.
I don't love this api design where the caller to the constructor has to path both writer W & Optional PathBuf, and it is up to the caller to know if PathBuf is some, we will require that file to exist whenever serialize is called.
what's to stop a person from calling something that looks like
let f = File::create("foo.txt")?;
let mut f = BufWriter::new(f);
let logger = SbpLogger::new(SbpLogging::SBP, f, None);
}I don't really have a great suggestion here, but I think it may be better to design it as follows:
pub enum OutputDestination {
File(PathBuf)
}
#[derive(Debug)]
pub struct SbpLogger<W: Write> {
logger: SbpLogging,
out: W,
output_type: OutputDestination,
}
impl<W: Write> SbpLogger<W> {
pub fn new(logger: SbpLogging, out: W, output_type: OutputType) -> Self {
Self { logger, out, output_type }
}You might argue YAGNI, with the enum of one type. However, I think if you aren't going to need an enum here, I don't understand why you would need the new fn to take a generic writer & an optional PathBuf....
There was a problem hiding this comment.
hmm yeah, i wasn't sure if abstraction is even needed here since idk if we would ever want to emit this data anywhere else - uploading ? or stdout or something? (which might also introduce some funky logic as to how we want to reflect # of bytes written or check the success of those) so kinda went in between
this just kinda explicitly state a link for the write to the pathbufs to address the UI not being updated (oOoOo spooky inodes 👻)
but if we do, maybe just have a callback for specific logger types would be best instead of a pathbuf to allow more flexibility 🤔
maybe file lock would be nice or best to just keep this simply as sbp file logger with pathbuf without doing funky impl Write stuff
hmm how would you feel about just renaming this to SbpFileLogger instead and remove the generic stuff, since it's called sbp_logging_filepath 'filepaths' throughout the code
There was a problem hiding this comment.
that's fine but then I would also remove the option on the PathBuf
console_backend/src/output.rs
Outdated
| CompactFormatter, | ||
| ))) | ||
| } | ||
| if let Some(path) = &self.path { |
There was a problem hiding this comment.
I think using something like notify or watchexec is a good idea but may require a bigger refactor than this bug necessitates. At the same time I don't think this check actually prevents all the bugs you are trying to prevent here, and it may create some new ones. You are just checking if the path exists, which could happen if someone deleted the file and then recreated it, this check would pass, but the write_all path would drop.
Another issue is that if you rename the file with mv the code before this change will keep writing to the renamed file, whereas this change will cause an unnecessary error imo.
Maybe these are tradeoffs that are ok to make, I don't have the context here and the jira tickets are minimal
* pass format index as field * handle only init * only init once for interactive components * pylint * should only reflect counters when sbp is logging
|
will merge this back into #862 to finalize the ticket, |
|
Frontend and Release Workflow Started here |
3 participants
check before writing to file