<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 19, 2017 at 9:33 AM, Jun Wu <span dir="ltr"><<a href="mailto:quark@fb.com" target="_blank">quark@fb.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"># HG changeset patch<br>
# User Jun Wu <<a href="mailto:quark@fb.com" target="_blank">quark@fb.com</a>><br>
# Date 1495051320 25200<br>
# Wed May 17 13:02:00 2017 -0700<br>
# Node ID e59c24bd146290c910945e84ac27a7<wbr>cb4edbf4dd<br>
# Parent d1ddcac58ac7085b1b0238b74e3887<wbr>1343730c91<br>
# Available At <a href="https://bitbucket.org/quark-zju/hg-draft" rel="noreferrer" target="_blank">https://bitbucket.org/quark-zj<wbr>u/hg-draft</a><br>
# hg pull <a href="https://bitbucket.org/quark-zju/hg-draft" rel="noreferrer" target="_blank">https://bitbucket.org/quark-zj<wbr>u/hg-draft</a> -r e59c24bd1462<br>
localrepo: make it possible to use flock in a repo<br>
<br>
This patch adds a "flock" repo requirement, once set, Mercurial will attempt<br>
to use flock to lock the repo and store on supported platforms.<br>
<br>
The flock will be performed on directory, so no extra files will be created:<br>
<br>
wlock: flock(".hg")<br>
lock: flock(".hg/store")<br>
<br>
This makes it impossible to have deadlock or repo corruption (no /proc<br>
mount) when running hg inside different Linux pid namespaces with a shared<br>
filesystem.<br>
<br>
For concerns about repo corruption on network filesystem or portability,<br>
this should be safe because util.makelock will double-check filesystem type<br>
so nfs/cifs/sshfs and Windows will just error out correctly.<br>
<br>
Note: in weird setups, like .hg/store lives on NFS when .hg is on local<br>
disk, the lock is not safe. But we have broken locks on that setup already,<br>
so this patch is at least not making things worse. It even makes things<br>
better in local shared repo use-case [1].<br>
<br>
[1]: Consider two repos sharing a store: repo1/.hg and repo2/.hg and<br>
shared/.hg/store. We should really make "repo.lock()" write a lock file at<br>
"shared/.hg/store/lock" but not "repo1/.hg/lock" or "repo2/.hg/lock". With<br>
this patch, "shared/.hg/store" gets locked properly and there is no such<br>
problem.<br></blockquote><div><br></div><div>From a high level, this approach seems reasonable. There are some obvious missing components to the implementation:</div><div><br></div><div>* Docs for the new requirement in internals/requirements.txt</div><div>* Ability to create new repos with the requirement</div><div><br></div><div>I initially thought it surprising that we didn't check for flock support at repo open time as part of validating requirements. But your commit message explains that we'll error at lock time upon missing flock support. Since we don't have reader locks and I think it is user hostile to lock readers out of repos no longer supporting flock, I think this is fine. But there should be a test demonstrating the behavior of a flock requirement without flock support.<br></div><div><br></div><div>Also, I'm going to bikeshed the requirement name. Requirements are global and will persist for all of time. With that context in mind, "flock" is arguably too generic. What this feature/requirement actually resembles is "use flock() for the locking strategy used by the 'store' layout/requirement which implies use of flock() on the lock and wlock files." The meaning of the "flock" requirement can thus only be interpreted in the presence of another requirement, such as "store." After all, if there is a "store2" requirement, then "flock" likely takes on new meaning (since we'd likely use a different locking strategy). I believe requirements should be narrowly tailored to the exact feature they support. Otherwise if their meaning changes over time, that opens us up to bugs and possibly repo corruption. So I think a more appropriate requirement name is something like "store-flock" so the limits of its application (to the lock and wlock files for the "store" requirement) are explicit and not subject to reinterpretation. Semantically, this is probably ideally expressed as a sub-requirement. e.g. "store+flock." But I'd rather punt on that discussion until we have a few more requirements and the relationship between them is less clear.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py<br>
--- a/mercurial/localrepo.py<br>
+++ b/mercurial/localrepo.py<br>
@@ -260,4 +260,5 @@ class localrepository(object):<br>
'relshared',<br>
'dotencode',<br>
+ 'flock',<br>
}<br>
openerreqs = {<br>
@@ -367,4 +368,9 @@ class localrepository(object):<br>
self.requirements, self.sharedpath, vfsmod.vfs)<br>
self.spath = self.store.path<br>
+<br>
+ if 'flock' in self.requirements and self.spath == self.path:<br>
+ raise error.RepoError(<br>
+ _('flock requires store path and repo path to be different'))<br>
+<br>
self.svfs = self.store.vfs<br>
self.sjoin = self.store.join<br>
@@ -1335,5 +1341,5 @@ class localrepository(object):<br>
<br>
def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc,<br>
- inheritchecker=None, parentenvvar=None):<br>
+ inheritchecker=None, parentenvvar=None, flockpath=None):<br>
parentlock = None<br>
# the contents of parentenvvar are used by the underlying lock to<br>
@@ -1345,5 +1351,5 @@ class localrepository(object):<br>
acquirefn=acquirefn, desc=desc,<br>
inheritchecker=inheritchecker,<br>
- parentlock=parentlock)<br>
+ parentlock=parentlock, flockpath=flockpath)<br>
except error.LockHeld as inst:<br>
if not wait:<br>
@@ -1391,6 +1397,12 @@ class localrepository(object):<br>
return l<br>
<br>
+ if 'flock' in self.requirements:<br>
+ flockpath = ''<br>
+ else:<br>
+ flockpath = None<br>
+<br>
l = self._lock(self.svfs, "lock", wait, None,<br>
- self.invalidate, _('repository %s') % self.origroot)<br>
+ self.invalidate, _('repository %s') % self.origroot,<br>
+ flockpath=flockpath)<br>
self._lockref = weakref.ref(l)<br>
return l<br>
@@ -1429,9 +1441,14 @@ class localrepository(object):<br>
self._filecache['dirstate'].r<wbr>efresh()<br>
<br>
+ if 'flock' in self.requirements:<br>
+ flockpath = ''<br>
+ else:<br>
+ flockpath = None<br>
+<br>
l = self._lock(self.vfs, "wlock", wait, unlock,<br>
self.invalidatedirstate, _('working directory of %s') %<br>
self.origroot,<br>
inheritchecker=self._wlockchec<wbr>ktransaction,<br>
- parentenvvar='HG_WLOCK_<wbr>LOCKER')<br>
+ parentenvvar='HG_WLOCK_<wbr>LOCKER', flockpath=flockpath)<br>
self._wlockref = weakref.ref(l)<br>
return l<br>
diff --git a/tests/hghave.py b/tests/hghave.py<br>
--- a/tests/hghave.py<br>
+++ b/tests/hghave.py<br>
@@ -217,4 +217,18 @@ def has_fifo():<br>
return False<br>
<br>
+@check("flock", "flock on whitelisted filesystems")<br>
+def has_flock():<br>
+ try:<br>
+ import fcntl<br>
+ fcntl.flock<br>
+ except ImportError:<br>
+ return False<br>
+ from mercurial import util<br>
+ try:<br>
+ fstype = util.getfstype('.')<br>
+ except OSError:<br>
+ return False<br>
+ return fstype in util._flockfswhitelist<br>
+<br>
@check("killdaemons", 'killdaemons.py support')<br>
def has_killdaemons():<br>
diff --git a/tests/test-lock-badness.t b/tests/test-lock-badness.t<br>
--- a/tests/test-lock-badness.t<br>
+++ b/tests/test-lock-badness.t<br>
@@ -1,7 +1,15 @@<br>
+#testcases default useflock<br>
#require unix-permissions no-root no-windows<br>
<br>
+#if useflock<br>
+#require flock<br>
+#endif<br>
+<br>
Prepare<br>
<br>
$ hg init a<br>
+#if useflock<br>
+ $ echo flock >> a/.hg/requires<br>
+#endif<br>
$ echo a > a/a<br>
$ hg -R a ci -A -m a<br>
@@ -11,4 +19,7 @@ Prepare<br>
updating to branch default<br>
1 files updated, 0 files merged, 0 files removed, 0 files unresolved<br>
+#if useflock<br>
+ $ echo flock >> b/.hg/requires<br>
+#endif<br>
<br>
Test that raising an exception in the release function doesn't cause the lock to choke<br>
______________________________<wbr>_________________<br>
Mercurial-devel mailing list<br>
<a href="mailto:Mercurial-devel@mercurial-scm.org" target="_blank">Mercurial-devel@mercurial-scm.<wbr>org</a><br>
<a href="https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel" rel="noreferrer" target="_blank">https://www.mercurial-scm.org/<wbr>mailman/listinfo/mercurial-dev<wbr>el</a><br>
</blockquote></div><br></div></div>