Submitter | phabricator |
---|---|
Date | Nov. 2, 2019, 1:51 a.m. |
Message ID | <differential-rev-PHID-DREV-q52b6kx2yrlvivobahzr-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/42679/ |
State | Superseded |
Headers | show |
Comments
This revision is now accepted and ready to land. indygreg added a comment. indygreg accepted this revision. Thank you for tracking down this issue and for the detailed commit message! I agree there is a race between 2 processes creating the lock and that we need to verify the created lock once we have it to ensure we "won" the file write race. FWIW there are known issues with Mercurial's repository locking semantics. I believe we will need to break Mercurial's on-disk file layout to fix all the problems. But (hard-to-debug) issues like the one fixed in this patch do exist and can be fixed without breaking backwards compatibility. If this subject interests you, I suggest reading https://www.usenix.org/system/files/conference/osdi14/osdi14-paper-pillai.pdf which chronicles how many popular pieces of software fail to get filesystem consistency correct due to incorrect use of locks, `fsync()`, etc. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7199/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7199 To: valentin.gatienbaron, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
valentin.gatienbaron added a comment. Thanks for the review. Did something go wrong in a rebase? I'm confused as to why there's two `self.vfs.unlink` calls in the diff displayed by phabricator. I have read the paper you mention, and I have seen repositories get corrupted for these kinds of reasons (revlog index's length may get written without the corresponding data if the machine reboots, transaction commits but fncache gets truncated when one runs out of disk space). But I don't think it talks of locking problems in the absence of crashes, as is happening here. The ordering of filesystem operations in-memory is stricter. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7199/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7199 To: valentin.gatienbaron, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
Patch
diff --git a/mercurial/lock.py b/mercurial/lock.py --- a/mercurial/lock.py +++ b/mercurial/lock.py @@ -355,6 +355,9 @@ # held, or can race and break valid lock. try: with lock(self.vfs, self.f + b'.break', timeout=0): + locker = self._readlock() + if not self._lockshouldbebroken(locker): + return locker self.vfs.unlink(self.f) except error.LockError: return locker