utils: drop boost::interprocess::file

ken2812221 commented at 8:05 AM on August 3, 2018: contributor

boost::interprocess::file_lock cannot open the files that contain characters which cannot be parsed by the user's code page on Windows.

This PR is seperated from #13426 for easier review.

fanquake added the label Windows on Aug 3, 2018

fanquake added the label Utils/log/libs on Aug 3, 2018

DrahtBot commented at 10:40 AM on August 3, 2018: contributor

Note to reviewers: This pull request conflicts with the following ones:

#13878 (utils: Add fstream wrapper to allow to pass unicode filename on Windows by ken2812221)
#13866 (utils: Use _wfopen and _wfreopen on Windows by ken2812221)
#13746 (-masterdatadir for datadir bootstrapping by kallewoof)
#13743 (refactor: Replace std/boost::bind with lambda by ken2812221)
#13159 (Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) by practicalswift)

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 cross-referenced this on Aug 3, 2018 from issue Test for Windows encoding issue by ken2812221

DrahtBot cross-referenced this on Aug 3, 2018 from issue -masterdatadir for datadir bootstrapping by kallewoof

DrahtBot cross-referenced this on Aug 3, 2018 from issue Remove the boost/algorithm/string/case_conv.hpp dependency by l2a5b1

DrahtBot cross-referenced this on Aug 3, 2018 from issue [bugfix] Fix encoding issue for Windows by ken2812221

ken2812221 force-pushed on Aug 3, 2018

Empact commented at 1:16 PM on August 3, 2018: member

Seems like a good opportunity to drop boost::interprocess::file_lock altogether, no?

ken2812221 force-pushed on Aug 3, 2018

in src/fs.h:28 in 3b72520dbf outdated

  20 | @@ -19,6 +21,21 @@ namespace fs = boost::filesystem;
  21 |  namespace fsbridge {
  22 |      FILE *fopen(const fs::path& p, const char *mode);
  23 |      FILE *freopen(const fs::path& p, const char *mode, FILE *stream);
  24 | +
  25 | +    class FileLock
  26 | +    {
  27 | +    public:
  28 | +        FileLock(const fs::path& file);

Empact commented at 3:52 PM on August 3, 2018:

nit: could delete default and copy constructor

ken2812221 commented at 4:35 PM on August 3, 2018:

Fixed

in src/fs.cpp:58 in 3b72520dbf outdated

  41 | +    lock.l_type = F_WRLCK;
  42 | +    lock.l_whence = SEEK_SET;
  43 | +    lock.l_start = 0;
  44 | +    lock.l_len = 0;
  45 | +    return -1 != fcntl(fd, F_SETLK, &lock);
  46 | +}

Empact commented at 3:59 PM on August 3, 2018:

The relevant boost implementation, for reference:

inline bool try_acquire_file_lock(file_handle_t hnd, bool &acquired)
{
   struct ::flock lock;
   lock.l_type    = F_WRLCK;
   lock.l_whence  = SEEK_SET;
   lock.l_start   = 0;
   lock.l_len     = 0;
   int ret = ::fcntl(hnd, F_SETLK, &lock);
   if(ret == -1){
      return (errno == EAGAIN || errno == EACCES) ?
               acquired = false, true : false;
   }
   return (acquired = true);
}

https://www.boost.org/doc/libs/1_67_0/boost/interprocess/detail/os_file_functions.hpp

Should we distinguish between hard (e.g. EINVAL) and soft (EAGAIN, EACCES) failures, as they do? We could throw in the former case? https://www.gnu.org/software/libc/manual/html_node/File-Locks.html

in src/fs.cpp:97 in 3b72520dbf outdated

  63 | +    if (hFile == INVALID_HANDLE_VALUE) {
  64 | +        return false;
  65 | +    }
  66 | +    _OVERLAPPED overlapped = {0};
  67 | +    return LockFileEx((HANDLE)hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped);
  68 | +}

Empact commented at 4:03 PM on August 3, 2018:

The relevant boost implementation, for reference:

inline bool try_acquire_file_lock(file_handle_t hnd, bool &acquired)
{
   const unsigned long len = ((unsigned long)-1);
   winapi::interprocess_overlapped overlapped;
   std::memset(&overlapped, 0, sizeof(overlapped));
   if(!winapi::lock_file_ex
      (hnd, winapi::lockfile_exclusive_lock | winapi::lockfile_fail_immediately,
       0, len, len, &overlapped)){
      return winapi::get_last_error() == winapi::error_lock_violation ?
               acquired = false, true : false;

   }
   return (acquired = true);
}

https://www.boost.org/doc/libs/1_67_0/boost/interprocess/detail/os_file_functions.hpp

Similar question re. GetLastError?

Empact commented at 4:05 PM on August 3, 2018:

Also is the (HANDLE) cast necessary?

ken2812221 commented at 4:18 PM on August 3, 2018:

Fixed

ken2812221 force-pushed on Aug 3, 2018

ken2812221 commented at 4:22 PM on August 3, 2018: contributor

@Empact In my opinion, we deal catching exception and try_lock fail much the same when we use boost, so no need to handle the extra exception for now.

ken2812221 force-pushed on Aug 3, 2018

ken2812221 renamed this:
~~add unicode compatible file_lock for Windows~~
utils: drop boost::interprocess::file_lock
on Aug 3, 2018

DrahtBot cross-referenced this on Aug 3, 2018 from issue refactor: Replace boost::bind with std::bind by ken2812221

DrahtBot cross-referenced this on Aug 3, 2018 from issue gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows by ken2812221

Empact commented at 5:25 PM on August 3, 2018: member

@ken2812221 FYI boost does throw on the failures noted above:

inline bool file_lock::try_lock()
{
   bool result;
   if(!ipcdetail::try_acquire_file_lock(m_file_hnd, result)){
      error_info err(system_error_code());
      throw interprocess_exception(err);
   }
   return result;
}

namespace boost {
namespace interprocess {
inline int system_error_code() // artifact of POSIX and WINDOWS error reporting
{
   #if (defined BOOST_INTERPROCESS_WINDOWS)
   return winapi::get_last_error();
   #else
   return errno; // GCC 3.1 won't accept ::errno
   #endif
}

https://www.boost.org/doc/libs/1_67_0/boost/interprocess/sync/file_lock.hpp https://www.boost.org/doc/libs/1_46_1/boost/interprocess/errors.hpp

ken2812221 commented at 5:35 PM on August 3, 2018: contributor

@Empact I simply return false in my code at those conditions.

Empact commented at 5:42 PM on August 3, 2018: member

Yeah so the net effect is the loss of this message: return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());

Certainly not a big deal but breadcrumbs can mean the difference between an easy fix and a maddening mystery.

ken2812221 force-pushed on Aug 3, 2018

ken2812221 commented at 6:32 PM on August 3, 2018: contributor

@Empact Updated to get error messages.

ken2812221 cross-referenced this on Aug 3, 2018 from issue Filename and command line encoding issue on Windows by ken2812221

DrahtBot cross-referenced this on Aug 3, 2018 from issue utils: Use _wfopen and _wfreopen on Windows by ken2812221

DrahtBot cross-referenced this on Aug 3, 2018 from issue Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact

DrahtBot cross-referenced this on Aug 3, 2018 from issue Include tinyformat as a subtree by Empact

DrahtBot cross-referenced this on Aug 3, 2018 from issue Inline i64tostr and itostr by Empact

DrahtBot cross-referenced this on Aug 3, 2018 from issue PSBT key path cleanups by sipa

in src/fs.cpp:67 in 574c4b7216 outdated

  62 | +{
  63 | +    if (hFile == INVALID_HANDLE_VALUE) {
  64 | +        return false;
  65 | +    }
  66 | +    _OVERLAPPED overlapped = {0};
  67 | +    return LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped);

donaloconnor commented at 9:30 PM on August 3, 2018:

The CreateFileW with 0 sharing effectively opens the file as exclusive (locking it). It feels the TryLock() here is redundant because we don't require locking of specific bytes.

Perhaps the TryLock() should be the one doing the CreateFileW. Apologies if I've misunderstood something here :-)

ken2812221 commented at 11:03 PM on August 3, 2018:

Fixed

ken2812221 force-pushed on Aug 3, 2018

in src/fs.cpp:24 in 3b8ced6a4d outdated

  20 | @@ -12,4 +21,82 @@ FILE *freopen(const fs::path& p, const char *mode, FILE *stream)
  21 |      return ::freopen(p.string().c_str(), mode, stream);
  22 |  }
  23 |  
  24 | +#ifndef WIN32
  25 | +
  26 | +std::string GetErrorReason() {

Empact commented at 3:28 PM on August 4, 2018:

nit: static

in src/fs.cpp:61 in 3b8ced6a4d outdated

  58 | +    }
  59 | +    return true;
  60 | +}
  61 | +#else
  62 | +
  63 | +std::string GetErrorReason() {

Empact commented at 3:29 PM on August 4, 2018:

nit: static

in src/util.cpp:160 in 3b8ced6a4d outdated

 169 | -        }
 170 | -    } catch (const boost::interprocess::interprocess_exception& e) {
 171 | -        return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());
 172 | +    auto lock = MakeUnique<fsbridge::FileLock>(pathLockFile);
 173 | +    if (!lock->TryLock()) {
 174 | +        return error("Error while attempting to lock directory %s: %s", directory.string(), lock->GetReason());

Empact commented at 3:44 PM on August 4, 2018:

For reference, how these are logged by the callers: https://github.com/bitcoin/bitcoin/blob/2b67354aa584c4aabae049a67767ac7b70e2d01a/src/init.cpp#L1183-L1185

https://github.com/bitcoin/bitcoin/blob/2b67354aa584c4aabae049a67767ac7b70e2d01a/src/wallet/db.cpp#L136-L139

DrahtBot cross-referenced this on Aug 4, 2018 from issue utils: Add fstream wrapper to allow to pass unicode filename on Windows by ken2812221

DrahtBot cross-referenced this on Aug 5, 2018 from issue utils: Make fs::path::string() always return utf-8 string on Windows by ken2812221

ken2812221 force-pushed on Aug 5, 2018

ken2812221 commented at 3:30 PM on August 5, 2018: contributor

Update: use std codecvt instead of custom converter.

ken2812221 force-pushed on Aug 18, 2018

ken2812221 force-pushed on Aug 21, 2018

laanwj commented at 12:33 PM on August 27, 2018: member

lightly tested ACK 80119487840db0c3944a89f92772660ee594c4c3

tested on-

Linux Ubuntu 18.04 (✅)
OpenBSD 6.3 (✅)
FreeBSD 11.2 (✅ )

in src/fs.cpp:53 in 8011948784 outdated

  48 | +    struct flock lock;
  49 | +    lock.l_type = F_WRLCK;
  50 | +    lock.l_whence = SEEK_SET;
  51 | +    lock.l_start = 0;
  52 | +    lock.l_len = 0;
  53 | +    if (-1 == fcntl(fd, F_SETLK, &lock)) {

laanwj commented at 12:35 PM on August 27, 2018:

would prefer simply fcntl(fd, F_SETLK, &lock) == -1 (more consistent with the other if statements in the code)

ken2812221 commented at 4:56 PM on August 27, 2018:

fixed

add unicode compatible file_lock for Windows

boost::interprocess::file_lock cannot open the files that contain characters which cannot be parsed by the user's code page on Windows.
This commit add a new class to handle those specific file for Windows.

1661a472b8

ken2812221 force-pushed on Aug 27, 2018

laanwj merged this on Aug 29, 2018

laanwj closed this on Aug 29, 2018

laanwj referenced this in commit 2ddce35abc on Aug 29, 2018

ken2812221 deleted the branch on Aug 29, 2018

Bushstar cross-referenced this on Sep 4, 2018 from issue commits from bitcoin/master by Bushstar

ken2812221 cross-referenced this on Sep 25, 2018 from issue [bugfix] wallet: Fix duplicate fileid detection by ken2812221

ken2812221 cross-referenced this on Oct 7, 2018 from issue utils: Fix broken Windows filelock by ken2812221

sipa referenced this in commit b2863c0685 on Oct 20, 2018

Warrows referenced this in commit e77d3053b0 on Oct 14, 2019

Warrows referenced this in commit 08d61194c8 on Nov 23, 2019

fanquake referenced this in commit b80ee23824 on May 11, 2020

fanquake cross-referenced this on May 11, 2020 from issue doc: Increase minimum required GCC to 5.1 by fanquake

kwvg referenced this in commit 6af12a6652 on May 19, 2021

kwvg referenced this in commit ac51437ad8 on May 19, 2021

kwvg cross-referenced this on May 19, 2021 from issue Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports by kwvg

kwvg referenced this in commit b571df7063 on May 20, 2021

kwvg referenced this in commit c9e1d132ee on May 20, 2021

kwvg referenced this in commit fc96dd7b31 on May 20, 2021

kwvg referenced this in commit 96eaef33ee on May 20, 2021

PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021

furszy cross-referenced this on Jun 24, 2021 from issue Solve filename and command line encoding issues on Windows by furszy

PastaPastaPasta referenced this in commit 29b1fe8413 on Jun 27, 2021

PastaPastaPasta referenced this in commit 6dc7af4639 on Jun 28, 2021

PastaPastaPasta referenced this in commit a50d8102f7 on Jun 29, 2021

PastaPastaPasta referenced this in commit 6dbb69f262 on Jul 1, 2021

PastaPastaPasta referenced this in commit 2d55a82f7f on Jul 1, 2021

PastaPastaPasta referenced this in commit 52bb384a3c on Jul 1, 2021

random-zebra referenced this in commit 61a098a775 on Aug 5, 2021

vijaydasmp referenced this in commit 124cf306de on Sep 8, 2021

bitcoin locked this on Sep 8, 2021

utils: drop boost::interprocess::file_lock #13862