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: member

#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.

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:

 0inline bool try_acquire_file_lock(file_handle_t hnd, bool &acquired)
 1{
 2   struct ::flock lock;
 3   lock.l_type    = F_WRLCK;
 4   lock.l_whence  = SEEK_SET;
 5   lock.l_start   = 0;
 6   lock.l_len     = 0;
 7   int ret = ::fcntl(hnd, F_SETLK, &lock);
 8   if(ret == -1){
 9      return (errno == EAGAIN || errno == EACCES) ?
10               acquired = false, true : false;
11   }
12   return (acquired = true);
13}

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:

 0inline bool try_acquire_file_lock(file_handle_t hnd, bool &acquired)
 1{
 2   const unsigned long len = ((unsigned long)-1);
 3   winapi::interprocess_overlapped overlapped;
 4   std::memset(&overlapped, 0, sizeof(overlapped));
 5   if(!winapi::lock_file_ex
 6      (hnd, winapi::lockfile_exclusive_lock | winapi::lockfile_fail_immediately,
 7       0, len, len, &overlapped)){
 8      return winapi::get_last_error() == winapi::error_lock_violation ?
 9               acquired = false, true : false;
10
11   }
12   return (acquired = true);
13}

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

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

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

 0inline bool file_lock::try_lock()
 1{
 2   bool result;
 3   if(!ipcdetail::try_acquire_file_lock(m_file_hnd, result)){
 4      error_info err(system_error_code());
 5      throw interprocess_exception(err);
 6   }
 7   return result;
 8}
 9
10namespace boost {
11namespace interprocess {
12inline int system_error_code() // artifact of POSIX and WINDOWS error reporting
13{
14   #if (defined BOOST_INTERPROCESS_WINDOWS)
15   return winapi::get_last_error();
16   #else
17   return errno; // GCC 3.1 won't accept ::errno
18   #endif
19}

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.

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

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

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

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

kittywhiskers referenced this in commit ac51437ad8 on May 19, 2021

kittywhiskers referenced this in commit b571df7063 on May 20, 2021

kittywhiskers referenced this in commit c9e1d132ee on May 20, 2021

kittywhiskers referenced this in commit fc96dd7b31 on May 20, 2021

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

PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021

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

DrahtBot locked this on Sep 8, 2021

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