cpp: Call std::fflush() and ::fsync() before std::fclose() in FileWriter::end() #1343
Conversation
…end() I've observed periodic data loss writing MCAP files where I destroy the `McapWriter` (which triggers `FileWriter::end()` under the hood) then immediately call `std::rename()` to remove a `.active` extension from the written MCAP file. Most of the time this works, but maybe ~5% of my MCAPs are missing data at the end where the final bytes of the file are a partial Chunk record. From my reading it looks like calling `std::fclose()` immediately followed by `std::rename()` may not be safe if there are writes that haven't flushed to disk. This change adds additional calls to `std::fflush()` and `::fsync()` to guarantee the OS has delivered all pending writes to the physical media before `FileWriter::end()` returns, and hence before my `std::rename()` call.
jhurliman
requested review from
jtbandes,
james-rms and
indirectlylit
as code owners
jhurliman
changed the title
jhurliman
changed the title
| void FileWriter::end() { | ||
| if (file_) { | ||
| std::fflush(file_); | ||
| #if defined(_WIN32) || defined(_WIN64) |
There was a problem hiding this comment.
we have a different set of platform-detection ifdefs in visibility.hpp - we don't check for win64, we do check for cygwin.
From https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170, it looks like _WIN64 is redundant with _WIN32.
Also, ::fsync is only defined in unistd.h, which we don't include in mcap, so this code only works when built into another object that includes it. We probably need a conditional include at the top of the file.
There was a problem hiding this comment.
Similar with _fileno, which microsoft says is defined in stdio.h - we only include cstdio, which may resolve to the same thing? I haven't tested. https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fileno?view=msvc-170
Changelog
FileWriter::end()returnsDocs
None
Description
I've observed periodic data loss writing MCAP files where I destroy the
McapWriter(which triggersFileWriter::end()under the hood) then immediately callstd::rename()to remove a.activeextension from the written MCAP file. Most of the time this works, but maybe ~5% of my MCAPs are missing data at the end where the final bytes of the file are a partial Chunk record.From my reading it looks like calling
std::fclose()immediately followed bystd::rename()may not be safe if there are writes that haven't flushed to disk. This change adds additional calls tostd::fflush()and::fsync()to guarantee the OS has delivered all pending writes to the physical media beforeFileWriter::end()returns, and hence before mystd::rename()call.This change was manually tested, I am still monitoring but haven't seen any partial writes since deploying this fix locally.