Skip to content

util: explicitly close all AutoFiles that have been written#29307

Merged
achow101 merged 4 commits intobitcoin:masterfrom
vasild:AutoFile_error_check
Jul 3, 2025
Merged

util: explicitly close all AutoFiles that have been written#29307
achow101 merged 4 commits intobitcoin:masterfrom
vasild:AutoFile_error_check

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Jan 24, 2024

fclose(3) may fail to flush the previously written data to disk, thus a failing fclose(3) is as serious as a failing fwrite(3).

Previously the code ignored fclose(3) failures. This PR improves that by changing all users of AutoFile that use it to write data to explicitly close the file and handle a possible error.


Other alternatives are:

  1. fflush(3) after each write to the file (and throw if it fails from the AutoFile::write() method) and hope that fclose(3) will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
  2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
  3. (this is implemented in the latest incarnation of this PR) Redesign AutoFile so that its destructor cannot fail. Adjust all its users 😭. For example, if the file has been written to, then require the callers to explicitly call the AutoFile::fclose() method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for FILE* which automatically closes the file when it goes out of scope and there are a lot of users of AutoFile.
  4. Pass a new callback function to the AutoFile constructor which will be called from the destructor to handle fclose() errors, as described in util: explicitly close all AutoFiles that have been written #29307 (comment). My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling AutoFile::fclose() instead of getting the AutoFile object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed fclose()).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 24, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29307.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hodlinator, l0rinc, achow101
Concept ACK ryanofsky
Stale ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #32748 (fees: prevent redundant estimates flushes by ismaelsadeeq)
  • #31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)
  • #31144 ([IBD] multi-byte block obfuscation by l0rinc)
  • #28792 (Embed default ASMap as binary dump header file by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20819752683

@maflcko
Copy link
Member

maflcko commented Jan 29, 2024

I don't think any care close-check needs to be done when reading from a file.

So what about removing the write method from AutoFile, and introduce a new derived class to add it back. This class could Assume that the file was flushed/closed before the destructor is called?

@vasild
Copy link
Contributor Author

vasild commented Feb 4, 2024

I don't think any care close-check needs to be done when reading from a file.

That is my understanding too.

So what about removing the write method from AutoFile, and introduce a new derived class to add it back. This class could Assume that the file was flushed/closed before the destructor is called?

Assume is more for code correctness, not for external errors (like IO error). If it does not fail during testing and on CI, that does not mean IO errors are absent and will not occur on the users' machines. Given that Assume() is removed from production builds, it still means that file corruption could remain unnoticed on live environments, like now on master.

@maflcko
Copy link
Member

maflcko commented Feb 5, 2024

This class could Assume that the file was flushed/closed before the destructor is called?

Assume is more for code correctness, not for external errors (like IO error).

The assumption would be that the close was called before the destructor is called. This is a code correctness question and seems like the perfect fit for Assume. That is, if the Assume fails, the code was missing a close, and the code must be changed to fix the internal bug. The Assume does not in any way check for IO errors.

@vasild
Copy link
Contributor Author

vasild commented Feb 5, 2024

Then I misunderstood this:

This class could Assume that the file was flushed/closed before the destructor is called

How?

@maflcko
Copy link
Member

maflcko commented Feb 5, 2024

AutoFile holds a file, which will be nullptr when it is closed. So if the file is nullptr, then it can be assumed to be flushed.

@vasild
Copy link
Contributor Author

vasild commented Feb 5, 2024

How/when would the file be closed?

@maflcko
Copy link
Member

maflcko commented Feb 6, 2024

How/when would the file be closed?

An AutoFile can be closed by calling the fclose() method in the code.

@vasild
Copy link
Contributor Author

vasild commented Feb 6, 2024

Ok, the users of the class then have to explicitly close. That is 1.4. from the OP, I elaborated it with more words.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20850328376

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024
Explicit callers of `AutoFile::fclose()` should check whether
`fclose(3)` failed because this may be due to a failure to
flush written data to disk. Such failures after writing to
a file should be treated as failures to `fwrite(3)`.

Github-Pull: bitcoin#29307
Rebased-From: e9abf95ab52f1f2d6f73ecca63133c29c253b7d9
Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024
`fread(3)` does not distinguish between end-of-file and error,
and callers must use `feof(3)` and `ferror(3)` to determine which
occurred.

Github-Pull: bitcoin#29307
Rebased-From: 55439903212dcee6af09da7393a41e3754501b37
@achow101
Copy link
Member

achow101 commented Apr 9, 2024

Seems like there is some discussion in the description that is a better fit for a brainstorming issue.

@DrahtBot DrahtBot marked this pull request as draft April 23, 2024 06:50
Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review c7b68dceb29ef67796f7edf79a161bbda766169c

This is a clear incremental improvement over what is on master.

Thanks for taking my suggestions!

Don't foresee any further issues preventing an A-C-K beyond the inline comments below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this specific case of mempool_persist.cpp/DumpMempool my variant simply had:

    FileWriter file{mockable_fopen_function(file_fspath, "wb"), [] (int err) {
        Assume(std::uncaught_exceptions() > 0); // Only expected when exception is thrown before fclose() below.
    }};

Which would serve to enforce that we either already explicitly fclose()d the file and checked errno further down in the function (leading to the lambda not being called upon destruction), or that an exception was triggered before reaching that far (in which case the Assume() in the lambda succeeds). (This assumes that the thrown exception is worse than the failure to close the file, which may not always be true).

For other cases I ended up with scopes and extra variables to check for failures, like in your examples in the comment above. Agree that it would be nice to find something more elegant... and until we do, your current approach in this PR seems okay.

nit: I still think the user-provided lambda in destructor approach merits an alternative 4 in the PR description.

src/flatfile.cpp Outdated
Comment on lines 74 to 75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to handle this in the callers, probably something along these lines:

diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index 6b47a0fa0a..332e700995 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -227,10 +227,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
 
     // Pre-allocate sufficient space for filter data.
     bool out_of_space;
-    m_filter_fileseq->Allocate(pos, data_size, out_of_space);
+    const size_t allocated_size = m_filter_fileseq->Allocate(pos, data_size, out_of_space);
     if (out_of_space) {
         LogPrintf("%s: out of disk space\n", __func__);
         return 0;
+    } else if (allocated_size < data_size) {
+        LogError("Failed to allocate %u for file %s.", data_size, pos.ToString());
+        return 0;
     }
 
     AutoFile fileout{m_filter_fileseq->Open(pos)};
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index a344e13c89..3ff3aa9122 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -873,6 +873,9 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
     if (out_of_space) {
         m_opts.notifications.fatalError(_("Disk space is too low!"));
         return {};
+    } else if (bytes_allocated < nAddSize) {
+        m_opts.notifications.fatalError(_("Failed allocating disk space!"));
+        return {};
     }
     if (bytes_allocated != 0 && IsPruneMode()) {
         m_check_for_pruning = true;
@@ -918,6 +921,8 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
     size_t bytes_allocated = m_undo_file_seq.Allocate(pos, nAddSize, out_of_space);
     if (out_of_space) {
         return FatalError(m_opts.notifications, state, _("Disk space is too low!"));
+    } else if (bytes_allocated < nAddSize) {
+        return FatalError(m_opts.notifications, state, _("Failed allocating disk space!"));
     }
     if (bytes_allocated != 0 && IsPruneMode()) {
         m_check_for_pruning = true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that callers of Allocate() can do better error handling. That seems to be out of the scope of this PR because this PR does not change the semantic of Allocate(). Also in master it could return 0 and have out_of_space == false. That means an error has occurred, e.g. it would do that if it cannot open the file (in master). Now it will also do it if it cannot close the file.

@vasild
Copy link
Contributor Author

vasild commented Jun 11, 2025

c7b68dceb2...004ec6968e: rebase and address suggestion

@vasild
Copy link
Contributor Author

vasild commented Jun 13, 2025

004ec6968e...f8a7580a08: rebase due to conflicts

Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f8a7580a0816586f75ba9b0e79300c893d362f9f

Mainly changes AutoFile to encourage judicious error checking upon file closure, surfacing failures to complete writing files to disk.

Several alternatives have been considered (listed in PR description) and I agree this is probably the best option for now.

Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f8a7580a0816586f75ba9b0e79300c893d362f9f

Changes since last review:

  • adjust AutoFile parameter move semantics;
  • log unsuccessful closes as a last attempt;
  • add m_was_written to Truncate;
  • & explanatory comments.
Details
diff --git a/src/flatfile.cpp b/src/flatfile.cpp
index 388b30efae..df6596e940 100644
--- a/src/flatfile.cpp
+++ b/src/flatfile.cpp
@@ -46,7 +46,9 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const
     }
     if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
         LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
-        fclose(file);
+        if (fclose(file) != 0) {
+            LogError("Unable to close file %s", fs::PathToString(path));
+        }
         return nullptr;
     }
     return file;
@@ -68,7 +70,10 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
             if (file) {
                 LogDebug(BCLog::VALIDATION, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
                 AllocateFileRange(file, pos.nPos, inc_size);
-                fclose(file);
+                if (fclose(file) != 0) {
+                    LogError("Cannot close file %s%05u.dat after extending it with %u bytes", m_prefix, pos.nFile, new_size);
+                    return 0;
+                }
                 return inc_size;
             }
         } else {
@@ -86,17 +91,24 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) const
         return false;
     }
     if (finalize && !TruncateFile(file, pos.nPos)) {
-        fclose(file);
         LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
+        if (fclose(file) != 0) {
+            LogError("Failed to close file %d", pos.nFile);
+        }
         return false;
     }
     if (!FileCommit(file)) {
-        fclose(file);
         LogError("%s: failed to commit file %d\n", __func__, pos.nFile);
+        if (fclose(file) != 0) {
+            LogError("Failed to close file %d", pos.nFile);
+        }
         return false;
     }
     DirectoryCommit(m_dir);
 
-    fclose(file);
+    if (fclose(file) != 0) {
+        LogError("Failed to close file %d after flush", pos.nFile);
+        return false;
+    }
     return true;
 }
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 557ddaceab..81f42827d9 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -961,6 +961,7 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
                 // Write undo data & checksum
                 fileout << blockundo << hasher.GetHash();
             }
+            // BufferedWriter will flush pending data to file when fileout goes out of scope.
         }
 
         // Make sure that the file is closed before we call `FlushUndoFile`.
diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp
index 2551a43166..d4d9263973 100644
--- a/src/node/mempool_persist.cpp
+++ b/src/node/mempool_persist.cpp
@@ -201,8 +201,10 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
         LogInfo("Writing %d unbroadcast transactions to file.\n", unbroadcast_txids.size());
         file << unbroadcast_txids;
 
-        if (!skip_file_commit && !file.Commit())
+        if (!skip_file_commit && !file.Commit()) {
+            (void)file.fclose();
             throw std::runtime_error("Commit failed");
+        }
         if (file.fclose() != 0) {
             throw std::runtime_error(
                 strprintf("Error closing %s: %s", fs::PathToString(file_fspath), SysErrorString(errno)));
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 4f5a6b8434..be953cefde 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -83,7 +83,7 @@ UniValue WriteUTXOSnapshot(
     CCoinsViewCursor* pcursor,
     CCoinsStats* maybe_stats,
     const CBlockIndex* tip,
-    AutoFile& afile,
+    AutoFile&& afile,
     const fs::path& path,
     const fs::path& temppath,
     const std::function<void()>& interruption_point = {});
@@ -3115,7 +3115,14 @@ static RPCHelpMan dumptxoutset()
         }
     }
 
-    UniValue result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
+    UniValue result = WriteUTXOSnapshot(*chainstate,
+                                        cursor.get(),
+                                        &stats,
+                                        tip,
+                                        std::move(afile),
+                                        path,
+                                        temppath,
+                                        node.rpc_interruption_point);
     fs::rename(temppath, path);
 
     result.pushKV("path", path.utf8string());
@@ -3167,7 +3174,7 @@ UniValue WriteUTXOSnapshot(
     CCoinsViewCursor* pcursor,
     CCoinsStats* maybe_stats,
     const CBlockIndex* tip,
-    AutoFile& afile,
+    AutoFile&& afile,
     const fs::path& path,
     const fs::path& temppath,
     const std::function<void()>& interruption_point)
@@ -3244,12 +3251,19 @@ UniValue WriteUTXOSnapshot(
 UniValue CreateUTXOSnapshot(
     node::NodeContext& node,
     Chainstate& chainstate,
-    AutoFile& afile,
+    AutoFile&& afile,
     const fs::path& path,
     const fs::path& tmppath)
 {
     auto [cursor, stats, tip]{WITH_LOCK(::cs_main, return PrepareUTXOSnapshot(chainstate, node.rpc_interruption_point))};
-    return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, afile, path, tmppath, node.rpc_interruption_point);
+    return WriteUTXOSnapshot(chainstate,
+                             cursor.get(),
+                             &stats,
+                             tip,
+                             std::move(afile),
+                             path,
+                             tmppath,
+                             node.rpc_interruption_point);
 }
 
 static RPCHelpMan loadtxoutset()
diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h
index b42fe96fd3..0e42bed947 100644
--- a/src/rpc/blockchain.h
+++ b/src/rpc/blockchain.h
@@ -51,7 +51,7 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
 UniValue CreateUTXOSnapshot(
     node::NodeContext& node,
     Chainstate& chainstate,
-    AutoFile& afile,
+    AutoFile&& afile,
     const fs::path& path,
     const fs::path& tmppath);
 
diff --git a/src/streams.cpp b/src/streams.cpp
index 3c46530990..2c873e50d3 100644
--- a/src/streams.cpp
+++ b/src/streams.cpp
@@ -119,6 +119,7 @@ bool AutoFile::Commit()
 
 bool AutoFile::Truncate(unsigned size)
 {
+    m_was_written = true;
     return ::TruncateFile(m_file, size);
 }
 
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
index eebf504d98..d5100d15b1 100644
--- a/src/test/util/chainstate.h
+++ b/src/test/util/chainstate.h
@@ -47,8 +47,11 @@ CreateAndActivateUTXOSnapshot(
     FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
     AutoFile auto_outfile{outfile};
 
-    UniValue result = CreateUTXOSnapshot(
-        node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); // Will close auto_outfile.
+    UniValue result = CreateUTXOSnapshot(node,
+                                         node.chainman->ActiveChainstate(),
+                                         std::move(auto_outfile), // Will close auto_outfile.
+                                         snapshot_path,
+                                         snapshot_path);
     LogPrintf(
         "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write());
 

I have checked remaining explicit closes, couldn't find any other one that was an AutoFile, but was wondering if we should maybe check these as well:

I have also rebased the PR locally and ran all tests.

hodlinator and others added 4 commits June 16, 2025 15:20
Have `WriteUTXOSnapshot()` take rvalue reference to make it obvious that
it takes ownership of the file.
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).

So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
If an `AutoFile` has been written to, then expect callers to have closed
it explicitly via the `AutoFile::fclose()` method. This is because if
the destructor calls `std::fclose()` and encounters an error, then it
is too late to indicate this to the caller in a meaningful way.
@vasild
Copy link
Contributor Author

vasild commented Jun 16, 2025

f8a7580a08...c10e382d2a: address suggestions

Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK c10e382

Only minor changes in response to feedback since first ACK (#29307 (review))

@l0rinc
Copy link
Contributor

l0rinc commented Jun 16, 2025

ACK c10e382

@vasild, what do you think about the remaining fclose calls I mentioned above?

@vasild
Copy link
Contributor Author

vasild commented Jun 17, 2025

what do you think about the remaining fclose calls I mentioned above?

Here is my take of each one:

std::fclose(created);

In this case the file is opened or created and closed immediately, without writing to it. So this cannot cause a data corruption. Still, would be sensible to check fclose() for error and return LockResult::ErrorWrite if it fails.


and

The file is opened for reading only. Maybe it would be better to check fclose() errors and treat them as read errors, but as above, an error from fclose() cannot be an indication that the data we wrote to the disk did not make it to the storage.


fclose(file);

This code is beyond me, I don't get it. It opens a file, does fsync() without writing anything and closes the file.

The fsync() function shall request that all data for the open file descriptor named by fildes is to be transferred to the storage device associated with the file described by fildes.

There is no data associated with that file descriptor, so what is the point!? Now, the same file might be opened at the same time by another piece of code, but that would be a different file descriptor. Indeed DirectoryCommit() is used in such a way that some of the files it operates on will be opened by the DirectoryCommit()'s caller FlatFileSeq::Flush(). So what happens in FlatFileSeq::Flush() is roughly:

  1. open the file under file descriptor 1
  2. fsync() or fdatasync() on file descriptor 1 via FileCommit()
  3. DirectoryCommit(): open the file directory under file descriptor 2, fsync() file descriptor 2, close file descriptor 2
  4. close file descriptor 1

I am puzzled 😕 🤕 edit: now it makes sense, DirectoryCommit() syncs the directory itself, not the file, I was blind. So that fclose() better be checked for an error. This was added in #14501

Comment on lines +82 to +85
if (fileout.fclose() != 0) {
const int errno_save{errno};
remove(pathTmp);
LogError("Failed to close file %s after commit: %s", fs::PathToString(pathTmp), SysErrorString(errno_save));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this. Commit is stronger than fclose, so I fail to see how this could ever fail on a real and normal system.

Forcing devs to write verbose and dead code doesn't seem ideal. It would be better to just treat Commit as m_was_written=false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, would make sense to set m_was_written=false when we return true from AutoFile::Commit(). Maybe rename m_was_written to m_pending_output. Then this last if-block could be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename m_was_written to m_pending_output

Or just m_dirty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to ignore the return value of fileout.fclose() like in master and assume it must have succeeded? Note that the errors of fclose() are a superset of the fflush() errors. In other words, fclose() may fail for errors other than fflush() errors. From fclose man page:

   The fclose() function may also fail and set errno for any of the errors specified for  the  routines  close(2), write(2), or fflush(3).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that once the file is synced, subsequent IO errors shouldn't matter, because the file was already synced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to be on the safe side and handle fclose() errors, it is just a few extra lines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a Commit is not possible without closing the file, then Commit should close the file. Requiring all call sites of Commit to manually close the file seems verbose and less safe to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Commit() fails and (while failing) were to close the file internally as suggested, it should probably throw an exception after that so calling code doesn't try to continue using a closed AutoFile.

An alternative would be to have a AutoFile::CommitAndClose()-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the AutoFile?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to have a AutoFile::CommitAndClose()-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the AutoFile?

yes, that is what i was trying to suggest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current Commit() is good as it is. It does what it promises - flushes the file data (and checks that the flush succeeded). However a subsequent close is not guaranteed to succeed if commit succeeded before that because close does more than flush.

Comment on lines +102 to +103
if (fclose(file) != 0) {
LogError("Failed to close file %d", pos.nFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, etc ...

Copy link
Contributor

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am puzzled 😕 🤕 edit: now it makes sense, DirectoryCommit() syncs the directory itself, not the file, I was blind. So that fclose() better be checked for an error. This was added in #14501

Might need to consider networked FS directory behavior when adding errors (#14501 (comment))?

Comment on lines +82 to +85
if (fileout.fclose() != 0) {
const int errno_save{errno};
remove(pathTmp);
LogError("Failed to close file %s after commit: %s", fs::PathToString(pathTmp), SysErrorString(errno_save));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, would make sense to set m_was_written=false when we return true from AutoFile::Commit(). Maybe rename m_was_written to m_pending_output. Then this last if-block could be removed.

@achow101
Copy link
Member

achow101 commented Jul 3, 2025

ACK c10e382

Comment on lines +49 to +50
if (fclose(file) != 0) {
LogError("Unable to close file %s", fs::PathToString(path));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no write has happened, and an error is already logged. Seems redundant to log even more errors?

fclose(file);
LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
if (fclose(file) != 0) {
LogError("Failed to close file %d", pos.nFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. if truncation failed and is logged as error, i don't think it matters to log another error whether or not closing has failed as well.

@fanquake
Copy link
Member

fanquake commented Jul 8, 2025

@vasild Are you planning on responding to the (remaining) outstanding comments here? Or opening a followup?

@vasild
Copy link
Contributor Author

vasild commented Jul 16, 2025

I prefer to always check whether a syscall failed even if it seems that it should always succeed. In this particular case, the "seems" is not very strong because close is doing more things than flush or truncate and those things could fail. So I think the current code is solid. I do not see a value in removing a check whether close failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants