refactor: flush after calling write_all#3614
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
a86aac7 to
94364d2
Compare
Robot Results
|
Bravo555
left a comment
There was a problem hiding this comment.
Looks alright, some flushes could maybe be syncs, but I'm not sure if it's necessary either way, so approving.
| while let Some(bytes) = response.chunk().await? { | ||
| writer.write_all(&bytes)?; | ||
| } | ||
| writer.flush()?; |
There was a problem hiding this comment.
note: this is an example where there's no functional change, because flush on a File is a noop, but still it's probably good to have for consistency or if somebody added a buffer and forgot to add a flush call.
If we actually want to ensure that all writes reach the file, I think sync_data or sync_all might be better here.
|
|
||
| self.internal_listener.write(writer).await?; | ||
| self.external_listener.write(writer).await?; | ||
| writer.flush().await?; |
There was a problem hiding this comment.
note: if the caller calls serialize multiple times, then adding flush here means some of the flushes will be redundant. The point is to flush the writer before it's dropped, so the caller should probably do it either way, but keeping it here is also fine.
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
3a96e3c to
e108a93
Compare
Proposed changes
Improve the file handling by:
BufWriterto improve file performancefs::write(...)instead of manually opening a fileThere are probably many cases here where it's not strictly necessary to do a flush (anywhere where we close a file before we rely on the data being persisted), but there are definitely quite a few cases where we this assumption doesn't hold for us.
Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments