utils: Fix broken Windows filelock

ken2812221 commented at 4:26 pm on October 7, 2018: contributor

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

ken2812221 commented at 5:04 pm on October 7, 2018: contributor

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 commented at 8:23 pm on October 7, 2018: contributor

Lgtm, nice test

fanquake added the label Windows on Oct 7, 2018

fanquake added the label Utils/log/libs on Oct 7, 2018

donaloconnor commented at 5:06 pm on October 8, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14426/commits/6d336e39dffb4951f09beedc478ba425c38c60b2

in src/fs.cpp:92 in 6d336e39df outdated

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

Empact commented at 1:30 am on October 9, 2018:

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

ken2812221 force-pushed on Oct 9, 2018

DrahtBot commented at 8:16 am on October 9, 2018: member

#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 commented at 7:40 pm on October 9, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14426/commits/474d022036ca2114750fc650f6712a99eda1bad4

in test/functional/feature_filelock.py:17 in 474d022036 outdated

12+class FilelockTest(BitcoinTestFramework):
13+    def set_test_params(self):
14+        self.setup_clean_chain = True
15+        self.num_nodes = 2
16+
17+    def get_node_output(self, *, index, ret_code_expected):

promag commented at 1:00 am on October 10, 2018:

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

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

ken2812221 commented at 3:22 pm on October 11, 2018:

Done

ken2812221 force-pushed on Oct 11, 2018

in test/functional/feature_filelock.py:37 in 851df19a73 outdated

32+        self.setup_clean_chain = True
33+        self.num_nodes = 2
34+
35+    def run_test(self):
36+        self.nodes[1].stop()
37+        self.log.info("Using datadir " + self.nodes[0].datadir)

practicalswift commented at 9:25 pm on October 14, 2018:

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

ken2812221 commented at 9:56 pm on October 14, 2018:

Done

ken2812221 force-pushed on Oct 14, 2018

jnewbery commented at 3:19 pm on October 18, 2018: member

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 force-pushed on Oct 18, 2018

ken2812221 commented at 4:20 pm on October 18, 2018: contributor

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 approved

ryanofsky commented at 4:22 pm on October 18, 2018: member

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 commented at 4:30 pm on October 18, 2018: member

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 commented at 4:40 pm on October 18, 2018: contributor

@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 force-pushed on Oct 18, 2018

ken2812221 commented at 5:12 pm on October 18, 2018: contributor

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

ken2812221 force-pushed on Oct 18, 2018

utils: Fix broken Windows filelock 369244f654

ken2812221 force-pushed on Oct 18, 2018

donaloconnor commented at 9:51 pm on October 18, 2018: contributor

@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 commented at 1:29 pm on October 19, 2018: member

utACK 369244f654c9e36b803e841eb30fd0aa2960087a

ryanofsky commented at 4:14 pm on October 19, 2018: member

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

sipa commented at 1:59 am on October 20, 2018: member

utACK 369244f654c9e36b803e841eb30fd0aa2960087a

sipa merged this on Oct 20, 2018

sipa closed this on Oct 20, 2018

sipa referenced this in commit b2863c0685 on Oct 20, 2018

ken2812221 deleted the branch on Oct 23, 2018

laanwj referenced this in commit 570eb7b130 on Apr 11, 2019

deadalnix referenced this in commit 2800cca736 on Jun 3, 2020

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

PastaPastaPasta referenced this in commit ffba4141b7 on Jun 27, 2021

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

PastaPastaPasta referenced this in commit dcf92c1d20 on Jun 28, 2021

PastaPastaPasta referenced this in commit a50d8102f7 on Jun 29, 2021

PastaPastaPasta referenced this in commit 92740dcd5f on Jun 29, 2021

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

PastaPastaPasta referenced this in commit 4670959d4a on Jul 1, 2021

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

PastaPastaPasta referenced this in commit 1b92905cdc 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

MarcoFalke locked this on Sep 8, 2021

utils: Fix broken Windows filelock #14426