Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils: Fix broken Windows filelock #14426

Merged
merged 1 commit into from Oct 20, 2018

Conversation

Projects
None yet
@ken2812221
Copy link
Member

commented Oct 7, 2018

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

@ken2812221

This comment has been minimized.

@ch4ot1c

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

Lgtm, nice test

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

utACK 6d336e3

src/fs.cpp Outdated
@@ -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)) {

This comment has been minimized.

Copy link
@Empact

Empact Oct 9, 2018

Member

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

@ken2812221 ken2812221 force-pushed the ken2812221:filelock-test branch 3 times, most recently Oct 9, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Reviewers, this pull request conflicts with the following ones:
  • #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

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

utACK 474d022

test/functional/feature_filelock.py Outdated
self.setup_clean_chain = True
self.num_nodes = 2

def get_node_output(self, *, index, ret_code_expected):

This comment has been minimized.

Copy link
@promag

promag Oct 10, 2018

Member

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)

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 11, 2018

Author Member

Done

@ken2812221 ken2812221 force-pushed the ken2812221:filelock-test branch 2 times, most recently Oct 11, 2018

test/functional/feature_filelock.py Outdated

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 14, 2018

Member

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

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 14, 2018

Author Member

Done

@ken2812221 ken2812221 force-pushed the ken2812221:filelock-test branch 2 times, most recently Oct 14, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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 ken2812221 force-pushed the ken2812221:filelock-test branch Oct 18, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

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
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@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 ken2812221 force-pushed the ken2812221:filelock-test branch Oct 18, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

@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 ken2812221 force-pushed the ken2812221:filelock-test branch 3 times, most recently Oct 18, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:filelock-test branch to 369244f Oct 18, 2018

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

utACK 369244f

@ryanofsky
Copy link
Contributor

left a comment

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

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

utACK 369244f

@sipa sipa merged commit 369244f into bitcoin:master Oct 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Oct 20, 2018

Merge #14426: utils: Fix broken Windows filelock
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

@ken2812221 ken2812221 deleted the ken2812221:filelock-test branch Oct 23, 2018

@Bushstar Bushstar referenced this pull request Apr 10, 2019

Merged

Avoid redefine warning #15782

laanwj added a commit that referenced this pull request Apr 11, 2019

Merge #15782: Avoid redefine warning
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.