utils: Fix broken Windows filelock #14426

ken2812221 · 2018-10-07T16:26:46Z

Fix broken filelock on Windows, also add a test for this. It's a regression introduced by #13862.

ken2812221 · 2018-10-07T17:04:47Z

For reference, the boost implementation is in https://github.com/boostorg/interprocess/blob/8b3621353017aa527a22124655c9458d0b64b358/include/boost/interprocess/detail/os_file_functions.hpp#L230-L243

ch4ot1c · 2018-10-07T20:23:40Z

Lgtm, nice test

donaloconnor · 2018-10-08T17:06:06Z

utACK 6d336e3

Empact · 2018-10-09T01:30:07Z

src/fs.cpp

@@ -89,7 +89,7 @@ bool FileLock::TryLock()
        return false;
    }
    _OVERLAPPED overlapped = {0};
-    if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, 0, 0, &overlapped)) {
+    if (!LockFileEx(hFile, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, 0, -1, -1, &overlapped)) {


nit: std::numeric_limits<DWORD>::max() or explicitly casting to unsigned might be more clear.

DrahtBot · 2018-10-09T08:16:55Z

Reviewers, this pull request conflicts with the following ones:

[bugfix] wallet: Fix duplicate fileid detection #14320 ([bugfix] wallet: Fix duplicate fileid detection by ken2812221)

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.

donaloconnor · 2018-10-09T19:40:03Z

utACK 474d022

promag · 2018-10-10T01:00:02Z

test/functional/feature_filelock.py

+        self.setup_clean_chain = True
+        self.num_nodes = 2
+
+    def get_node_output(self, *, index, ret_code_expected):


Move to node class as get_output? If that's controversial then extract to a module function for now? Then below:

_, output = get_node_output(node=self.nodes[1], ret_code_expected=1)

practicalswift · 2018-10-14T21:25:24Z

test/functional/feature_filelock.py

+
+    def run_test(self):
+        self.nodes[1].stop()
+        self.log.info("Using datadir " + self.nodes[0].datadir)


Specify string format arguments as logging function parameters to allow for lazy evaluation.

jnewbery · 2018-10-18T15:19:04Z

This seems like a reasonable change (although I know nothing about the windows API). LockFileEx documentation is here: https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex.

The test case is good, but it has the wrong file permissions and replicates functionality that is already available in the test framework. I've reimplemented it here: https://github.com/jnewbery/bitcoin/tree/pr14426.1 which uses the assert_start_raises_init_error() method. It's more concise, runs faster, also covers trying to open a wallet being used by a different bitcoind process, and I think is clearer. Feel free to take that commit in this PR.

ken2812221 · 2018-10-18T16:20:53Z

Thanks, I didn't know test framework that much, just trying to do what I want. Your commit looks good to me, I'll take it. The only thing I change is to skip wallet test if it's not compiled.

ryanofsky

utACK c7296a98e56a3a38730575c7aa7f524f10e2a0a9, which seems to be updated with John's test.

Although the LockFileEx documentation just says:

Locking a region that goes beyond the current end-of-file position is not an error

The LockFile documentation is more specific and says:

You can lock bytes that are beyond the end of the current file. This is useful to coordinate adding records to the end of a file

which I think explains why the fix works.

jnewbery · 2018-10-18T16:30:13Z

utACK c7296a98e56a3a38730575c7aa7f524f10e2a0a9.

I think the feature_notifications.py failure in appveyer is unrelated. The feature_filelock.py failure is due to a path slash mismatch: C:/Users\appveyor instead of C:\Users\appveyor.

donaloconnor · 2018-10-18T16:40:43Z

@ken2812221 should we explicitly call UnlockFile before closing the handle. The OS does unlock it but it's not instant potentially. Maybe safer to unlock before closing

ken2812221 · 2018-10-18T17:12:04Z

@donaloconnor I am not sure what condition would be unsafe if we don't unlock the file. Also, the boost implementation here only close file in deconstructor.

donaloconnor · 2018-10-18T21:51:08Z

@ken2812221 I did a test there across 2 processes to see if there's any weird race conditions with not unlocking before close and there's not. I thought being explicit might be better. Anyway no big deal.

tested ACK.

jnewbery · 2018-10-19T13:29:29Z

utACK 369244f

ryanofsky

utACK 369244f, only change is setting ErrorMatch.PARTIAL_REGEX flag in test.

sipa · 2018-10-20T01:59:22Z

utACK 369244f

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

0b3a654 Avoid redefine warning (Peter Bushnell) Pull request description: Wrap preprocessor definition of NOMINMAX in ifndef conditional to suppress warning when cross compiling Windows. `fs.cpp:6:0: warning: "NOMINMAX" redefined` `/usr/lib/gcc/x86_64-w64-mingw32/7.3-posix/include/c++/x86_64-w64-mingw32/bits/os_defines.h:45:0: note: this is the location of the previous definition #define NOMINMAX 1` #define NOMINMAX was introduced in the following merge. #14426 ACKs for commit 0b3a65: practicalswift: utACK 0b3a654 promag: utACK 0b3a654. Tree-SHA512: 0175195b88e63d3d44ffac2b8cc87ae7b285a45ed4e49605bca0cc82db073006c22024ef9c2f287980d357dac1099f798f1eeaa0bd75bb7a625919dc1632366c

Summary: 369244f654 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 Backport Core of [[bitcoin/bitcoin#14426 | PR14426]] Test Plan: `ninja check check-functional` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6334

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

0b3a654 Avoid redefine warning (Peter Bushnell) Pull request description: Wrap preprocessor definition of NOMINMAX in ifndef conditional to suppress warning when cross compiling Windows. `fs.cpp:6:0: warning: "NOMINMAX" redefined` `/usr/lib/gcc/x86_64-w64-mingw32/7.3-posix/include/c++/x86_64-w64-mingw32/bits/os_defines.h:45:0: note: this is the location of the previous definition #define NOMINMAX 1` #define NOMINMAX was introduced in the following merge. bitcoin#14426 ACKs for commit 0b3a65: practicalswift: utACK 0b3a654 promag: utACK 0b3a654. Tree-SHA512: 0175195b88e63d3d44ffac2b8cc87ae7b285a45ed4e49605bca0cc82db073006c22024ef9c2f287980d357dac1099f798f1eeaa0bd75bb7a625919dc1632366c