Re: [lmi] [lmi-commits] master 271a1b8 09/33: Note a deferred boost-migration question

[Vadim Zeitlin]

On Mon,  3 May 2021 08:15:51 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:

GC> branch: master
GC> commit 271a1b891f725a48be79e745ce8388e8f6021465
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     Note a deferred boost-migration question
GC> ---
GC>  path_utility.cpp | 1 +
GC>  1 file changed, 1 insertion(+)
GC> 
GC> diff --git a/path_utility.cpp b/path_utility.cpp
GC> index ee5d9d0..a0c273b 100644
GC> --- a/path_utility.cpp
GC> +++ b/path_utility.cpp
GC> @@ -253,6 +253,7 @@ fs::path serial_file_path
GC>  /// A try-block is necessary because fs::remove() can throw. The
GC>  /// postcondition is asserted explicitly at the end of the try-block
GC>  /// because that fs::remove() documentation is still unclear:
GC> +///   BOOST !! Is this still true of std::filesystem?
GC>  /// apparently it mustn't fail without throwing, yet it doesn't throw
GC>  /// on an operation that must fail, like removing a file that's locked
GC>  /// by another process as in the motivating example above.

 I'm not sure how to test this exhaustively, but the following simple test
program:
---------------------------------- >8 --------------------------------------
#include <iostream>
#include <filesystem>

namespace fs = std::filesystem;

int main(int argc, char* argv[])
{
    if (argc < 2) {
        std::cout << "Usage: " << argv[0] << " <path-to-remove> \n";
        return 1;
    }
    fs::path p{argv[1]};

    std::cout << "Removing file \"" << fs::absolute(p).string() << "\"\n";

    if (!fs::exists(p))
        std::cout << "(even though it doesn't seem to exist)\n";

    std::error_code ec;
    fs::remove(p, ec);
    if (ec)
        std::cerr << "Error: " << ec << "\n";
    else
        std::cout << "Successfully removed.\n";

    std::cout << "File " << (fs::exists(p) ? "still exists" : "doesn't exist") 
<< "\n";

    return ec ? 1 : 0;
}
---------------------------------- >8 --------------------------------------
behaves as expected for me, i.e. it either succeeds and the file doesn't
exist afterwards or it fails and the file still exists (it always succeeds
when used with a non-existing file).

 Notably I've tested it with a file open in Microsoft Excel and it returns
an error ("generic:32", which must correspond to Win32 standard error code
ERROR_SHARING_VIOLATION) and the file is untouched.

 So it doesn't seem to be useful to keep this assert. I'd also like to add
that, as always, there is an unavoidable race condition here: remove()
might succeed, but the assert still fail, because somebody recreates a file
with the same name in between them. But there is the same race condition
inherent in unique_filepath() anyhow because the same thing would have even
more time to happen between the call to remove() inside it and the call
using the new file name in its caller. So checking for the file not
existing here was never really useful, IMHO.

 If you'd like to remove it, I'd also avoid using try/catch and use the
overload taking the error code instead, resulting in the following patch
(which should be applicable with "git apply"):
---------------------------------- >8 --------------------------------------
>From fbd147cae12d9c69016ab43399cd407a1e8e8232 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@tt-solutions.com>
Date: Thu, 13 May 2021 19:25:22 +0200
Subject: [PATCH] Remove unnecessary assert and try/catch from
 unique_filepath()

No real changes, just simplify the code slightly and update a comment
which doesn't seem to be relevant any longer.
---
 path_utility.cpp | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/path_utility.cpp b/path_utility.cpp
index c379ef2cf..80f341398 100644
--- a/path_utility.cpp
+++ b/path_utility.cpp
@@ -267,13 +267,8 @@ std::string portable_filename(std::string const& 
original_filename)
 ///
 /// Implementation notes.
 ///
-/// A try-block is necessary because fs::remove() can throw. The
-/// postcondition is asserted explicitly at the end of the try-block
-/// because the fs::remove() documentation is still unclear:
-///   BOOST !! Is this still true of std::filesystem?
-/// apparently it mustn't fail without throwing, yet it doesn't throw
-/// on an operation that must fail, like removing a file that's locked
-/// by another process as in the motivating example above.
+/// fs::remove() returns an error if the file couldn't be actually
+/// deleted, as would be the case for a file locked by another program.
 ///
 /// For *nix, the catch-clause is not normally expected to be reached,
 /// and the alternative filename it devises might work no better than
@@ -292,12 +287,9 @@ std::string portable_filename(std::string const& 
original_filename)
         return filepath;
         }

-    try
-        {
-        fs::remove(filepath);
-        LMI_ASSERT(!fs::exists(filepath));
-        }
-    catch(std::exception const&)
+    std::error_code ec;
+    fs::remove(filepath, ec);
+    if(ec)
         {
         std::string basename  = filepath.stem().string();
         std::string const extension = filepath.extension().string();
--
2.31.0
---------------------------------- >8 --------------------------------------

 Please let me know if you'd like to do anything more/else here, thanks,
VZ

pgp7vHWWn8q1M.pgp
Description: PGP signature