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

ken2812221 · 2018-08-03T08:05:36Z

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.

DrahtBot · 2018-08-03T10:40:47Z

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

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

Empact · 2018-08-03T13:16:40Z

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

Empact · 2018-08-03T15:52:34Z

src/fs.h

+    class FileLock
+    {
+    public:
+        FileLock(const fs::path& file);


nit: could delete default and copy constructor

Empact · 2018-08-03T15:59:50Z

src/fs.cpp

+    lock.l_start = 0;
+    lock.l_len = 0;
+    return -1 != fcntl(fd, F_SETLK, &lock);
+}


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

Empact · 2018-08-03T16:03:30Z

src/fs.cpp

+    }
+    _OVERLAPPED overlapped = {0};
+    return LockFileEx((HANDLE)hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped);
+}


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?

Also is the (HANDLE) cast necessary?

ken2812221 · 2018-08-03T16:22:19Z

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

Empact · 2018-08-03T17:25:54Z

@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 · 2018-08-03T17:35:07Z

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

Empact · 2018-08-03T17:42:34Z

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 · 2018-08-03T18:32:14Z

@Empact Updated to get error messages.

donaloconnor · 2018-08-03T21:30:54Z

src/fs.cpp

+        return false;
+    }
+    _OVERLAPPED overlapped = {0};
+    return LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped);


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

Empact · 2018-08-04T15:28:52Z

src/fs.cpp

@@ -12,4 +21,82 @@ FILE *freopen(const fs::path& p, const char *mode, FILE *stream)
    return ::freopen(p.string().c_str(), mode, stream);
 }

+#ifndef WIN32
+
+std::string GetErrorReason() {


nit: static

Empact · 2018-08-04T15:29:03Z

src/fs.cpp

+}
+#else
+
+std::string GetErrorReason() {


nit: static

Empact · 2018-08-04T15:44:45Z

src/util.cpp

-        return error("Error while attempting to lock directory %s: %s", directory.string(), e.what());
+    auto lock = MakeUnique<fsbridge::FileLock>(pathLockFile);
+    if (!lock->TryLock()) {
+        return error("Error while attempting to lock directory %s: %s", directory.string(), lock->GetReason());


For reference, how these are logged by the callers:

bitcoin/src/init.cpp

Lines 1183 to 1185 in 2b67354

if (!LockDirectory(datadir, ".lock", probeOnly)) {

return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME)));

}

bitcoin/src/wallet/db.cpp

Lines 136 to 139 in 2b67354

if (!LockDirectory(pathIn, ".walletlock")) {

LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath);

return false;

}

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.

1661a47 add unicode compatible file_lock for Windows (Chun Kuan Lee) Pull request description: 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. Tree-SHA512: e240479cda65958bf6e1319840b83928b2b50da81d99f4f002fb3b62621370bcd4bcfacd2b8c0678c443a650d6ba53d9d12618b591e5bfd67ac14388a18fd822

369244f utils: Fix broken Windows filelock (Chun Kuan Lee) Pull request description: Fix broken filelock on Windows, also add a test for this. It's a regression introduced by #13862. Tree-SHA512: 15665b1930cf39ec71f3ab07def8e2897659f6fd4d2de749d63a5a8ec920e4a04282f12bc262f242b1b3d14d2dd9fa191ddbcf16a46fb927b5b2b14d9f6b5d01

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

Since the merge of bitcoin#13862, and the inclusion of the <codecvt> header, it hasn't been possible to compile bitcoind (targeting Windows) using GCC 4.8.x. The libstdc++ packaged with GCC didn't include the <codecvt> header until GCC 5.1. See the GCC 5 release notes: https://gcc.gnu.org/gcc-5/changes.html. Given that we'll be requiring a c++17 capable compiler at some point in the near future, and, assuming no-one else has noticed this, I think we can just adjust the docs to indicate that it's actually GCC 5.1+ that is required (I don't think we need to break it down per target).

Merge bitcoin#19660, bitcoin#19373, bitcoin#19841, bitcoin#13862, bitcoin#13866, bitcoin#17280, bitcoin#17682 and partial bitcoin#19326, bitcoin#14978: Auxiliary Backports

369244f utils: Fix broken Windows filelock (Chun Kuan Lee) Pull request description: Fix broken filelock on Windows, also add a test for this. It's a regression introduced by bitcoin#13862. Tree-SHA512: 15665b1930cf39ec71f3ab07def8e2897659f6fd4d2de749d63a5a8ec920e4a04282f12bc262f242b1b3d14d2dd9fa191ddbcf16a46fb927b5b2b14d9f6b5d01

63e0be6 [Remove] By-pass logprint-scanner restriction. (furszy) 280ced3 utils: Fix broken Windows filelock (Chun Kuan Lee) be89860 utils: Convert Windows args to utf-8 string (Chun Kuan Lee) e8cfa6e Call unicode API on Windows (Chun Kuan Lee) 1a02a8a tests: Add test case for std::ios_base::ate (Chun Kuan Lee) 2e57cd4 Move boost/std fstream to fsbridge (furszy) 9d8bcd4 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee) d59d48d utils: Convert fs error messages from multibyte to utf-8 (ken2812221) 9ef58cc Logging: use "fmterr" variable name for errors instead of general "e" that can be used by any other function. (furszy) dd94241 utils: Use _wfopen and _wreopen on Windows (Chun Kuan Lee) 3993641 add unicode compatible file_lock for Windows (Chun Kuan Lee) 48349f8 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: As the software is currently using the ANSI encoding on Windows, the user's language settings could affect the proper functioning of the node/wallet, to the point of not be able to open some non-ASCII name files and directories. This solves the Windows encoding issues, completing the entire bitcoin#13869 work path (and some other required backports). Enabling for example users that use non-english characters in directories and file names to be accepted. Backported PRs: * bitcoin#12422. * bitcoin#12630. * bitcoin#13862. * bitcoin#13866. * bitcoin#13877. * bitcoin#13878. * bitcoin#13883. * bitcoin#13884. * bitcoin#13886. * bitcoin#13888. * bitcoin#14192. * bitcoin#13734. * bitcoin#14426. This is built on top of other two PRs that i have open #2423 and #2369. Solves old issues #940 and #2163. TODO: * Backport `assert_start_raises_init_error` and `ErrorMatch` in TestNode` (bitcoin#12718) ACKs for top commit: Fuzzbawls: ACK 63e0be6 random-zebra: ACK 63e0be6 and merging... Tree-SHA512: cb1f7c23abb5b7b3af50bba18652cc2cad93fd7c2fca9c16ffd3fee34c4c152a3b666dfa87fe6b44c430064dcdee4367144dcb4a41203c91b0173b805bdb3d7d

369244f utils: Fix broken Windows filelock (Chun Kuan Lee) Pull request description: Fix broken filelock on Windows, also add a test for this. It's a regression introduced by bitcoin#13862. Tree-SHA512: 15665b1930cf39ec71f3ab07def8e2897659f6fd4d2de749d63a5a8ec920e4a04282f12bc262f242b1b3d14d2dd9fa191ddbcf16a46fb927b5b2b14d9f6b5d01

fanquake added Windows Utils/log/libs labels Aug 3, 2018

This was referenced Aug 3, 2018

Test for Windows encoding issue #13787

Merged

-masterdatadir for datadir bootstrapping #13746

Closed

Remove the boost/algorithm/string/case_conv.hpp dependency #13671

Merged

[bugfix] Fix encoding issue for Windows #13426

Closed

Empact reviewed Aug 3, 2018

View reviewed changes

ken2812221 changed the title ~~add unicode compatible file_lock for Windows~~ utils: drop boost::interprocess::file_lock Aug 3, 2018

This was referenced Aug 3, 2018

refactor: Replace boost::bind with std::bind #13743

Merged

gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows #13734

Merged

ken2812221 mentioned this pull request Aug 3, 2018

Filename and command line encoding issue on Windows #13869

Closed

12 tasks

This was referenced Aug 3, 2018

utils: Use _wfopen and _wfreopen on Windows #13866

Merged

Move src/tinyformat.h to src/tinyformat/tinyformat.h #13846

Closed

Include tinyformat as a subtree #13845

Closed

Inline i64tostr and itostr #13840

Closed

PSBT key path cleanups #13723

Merged

donaloconnor reviewed Aug 3, 2018

View reviewed changes

Empact reviewed Aug 4, 2018

View reviewed changes

src/fs.cpp Outdated

}

#else

std::string GetErrorReason() {

Copy link

Contributor

Empact Aug 4, 2018

Choose a reason for hiding this comment

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

nit: static

Empact reviewed Aug 4, 2018

View reviewed changes

DrahtBot mentioned this pull request Aug 4, 2018

utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

Merged

add unicode compatible file_lock for Windows

1661a47

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.

laanwj merged commit 1661a47 into bitcoin:master Aug 29, 2018

ken2812221 deleted the custom-filelock branch August 29, 2018 12:33

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

ken2812221 mentioned this pull request Sep 25, 2018

[bugfix] wallet: Fix duplicate fileid detection #14320

Merged

ken2812221 mentioned this pull request Oct 7, 2018

utils: Fix broken Windows filelock #14426

Merged

fanquake mentioned this pull request May 11, 2020

doc: Increase minimum required GCC to 5.1 #18942

Closed

kwvg pushed a commit to kwvg/dash that referenced this pull request May 19, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

6af12a6

kwvg added a commit to kwvg/dash that referenced this pull request May 19, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

ac51437

kwvg mentioned this pull request May 19, 2021

Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports dashpay/dash#4164

Merged

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

b571df7

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

c9e1d13

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

fc96dd7

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#13862: drop boost::interprocess::file_lock

96eaef3

furszy mentioned this pull request Jun 24, 2021

Solve filename and command line encoding issues on Windows PIVX-Project/PIVX#2449

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

	if (!LockDirectory(datadir, ".lock", probeOnly)) {
	return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), datadir.string(), _(PACKAGE_NAME)));
	}

	if (!LockDirectory(pathIn, ".walletlock")) {
	LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath);
	return false;
	}

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

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

Uh oh!

Conversation

ken2812221 commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Aug 3, 2018

Uh oh!

ken2812221 commented Aug 3, 2018

Uh oh!

Empact commented Aug 3, 2018

Uh oh!

ken2812221 commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ken2812221 commented Aug 3, 2018 •

edited

Loading

DrahtBot commented Aug 3, 2018 •

edited

Loading

Empact Aug 3, 2018 •

edited

Loading

ken2812221 commented Aug 3, 2018 •

edited

Loading