Patchwork [1,of,2,RFC] lock: support flock

login
register
mail settings
Submitter Jun Wu
Date May 19, 2017, 4:33 p.m.
Message ID <d1ddcac58ac7085b1b02.1495211591@x1c>
Download mbox | patch
Permalink /patch/20734/
State Changes Requested
Headers show

Comments

Jun Wu - May 19, 2017, 4:33 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1495048786 25200
#      Wed May 17 12:19:46 2017 -0700
# Node ID d1ddcac58ac7085b1b0238b74e38871343730c91
# Parent  763d7292569138886c3fdf179f7e688351bfb212
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r d1ddcac58ac7
lock: support flock

This patch makes it possible to use flock internally.

flock is more reliable in Linux's pid namespace use-case than file-existence
test, because it works without a /proc filesystem and does not have deadlock
issue if an hg process is killed.

Patch

diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -48,5 +48,10 @@  class lock(object):
     Typically used via localrepository.lock() to lock the repository
     store (.hg/store/) or localrepository.wlock() to lock everything
-    else under .hg/.'''
+    else under .hg/.
+
+    If flockpath is not None, flock will be used instead of file-existence
+    lock. The lock file will still be used to provide lock information but it
+    is not effective for locking purpose.
+    '''
 
     # lock is symlink on platforms that support it, file on others.
@@ -61,5 +66,6 @@  class lock(object):
 
     def __init__(self, vfs, file, timeout=-1, releasefn=None, acquirefn=None,
-                 desc=None, inheritchecker=None, parentlock=None):
+                 desc=None, inheritchecker=None, parentlock=None,
+                 flockpath=None):
         self.vfs = vfs
         self.f = file
@@ -72,4 +78,6 @@  class lock(object):
         self.parentlock = parentlock
         self._parentheld = False
+        self._flockpath = flockpath
+        self._flockfd = None
         self._inherited = False
         self.postrelease  = []
@@ -97,4 +105,11 @@  class lock(object):
         self.release()
 
+    def _getflockfd(self):
+        if self._flockfd is None:
+            if self._flockpath is not None:
+                self._flockfd = os.open(self.vfs.join(self._flockpath),
+                                        os.O_RDONLY)
+        return self._flockfd
+
     def _getpid(self):
         # wrapper around util.getpid() to make testing easier
@@ -127,9 +142,14 @@  class lock(object):
             retry -= 1
             try:
-                self.vfs.makelock(lockname, self.f)
+                self.vfs.makelock(lockname, self.f, self._getflockfd())
                 self.held = 1
             except (OSError, IOError) as why:
-                if why.errno == errno.EEXIST:
+                # EAGAIN: flock fails, EEXIST: non-flock fails
+                if why.errno in (errno.EEXIST, errno.EAGAIN):
                     locker = self._readlock()
+                    # for flock case, locker may be outdated but surely some
+                    # process is alive and taking the lock
+                    if why.errno == errno.EAGAIN and locker is None:
+                        locker = 'unknown'
                     if locker is None:
                         continue
@@ -163,4 +183,8 @@  class lock(object):
         Returns None if no lock exists, pid for old-style locks, and host:pid
         for new-style locks.
+
+        Note: in flock's case, _readlock() is a best-effort and could be
+        non-reliable after a makelock failure. It's still reliable if makelock
+        succeeded.
         """
         try:
@@ -172,4 +196,8 @@  class lock(object):
 
     def _testlock(self, locker):
+        if self._flockpath is not None:
+            # when flock is used, locker couldn't be dead (if it were, the OS
+            # will release its flock and we should not hit this code path)
+            return locker
         if locker is None:
             return None
@@ -260,4 +288,8 @@  class lock(object):
                     except OSError:
                         pass
+                    # release flock
+                    if self._flockfd is not None:
+                        os.close(self._flockfd)
+                        self._flockfd = None
             # The postrelease functions typically assume the lock is not held
             # at all.
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1080,4 +1080,7 @@  def checksignature(func):
 }
 
+# a whitelist of known filesystems where flock works reliably
+_flockfswhitelist = _hardlinkfswhitelist
+
 def copyfile(src, dest, hardlink=False, copystat=False, checkambig=False):
     '''copy a file, preserving mode and optionally other stat info like
@@ -1233,16 +1236,40 @@  if safehasattr(time, "perf_counter"):
     timer = time.perf_counter
 
-def makelock(info, pathname):
+def makelock(info, pathname, flockfd=None):
+    openflags = os.O_CREAT | os.O_WRONLY
+    # if flockfd is provided, use it as the source of truth of locking. flockfd
+    # should be on a same filesystem of pathname so the sanity check about
+    # filesystem type works.
+    if flockfd is not None:
+        fstype = getfstype(os.path.dirname(pathname)) or _('(unknown)')
+        if fstype not in _flockfswhitelist:
+            raise error.Abort(_('filesystem %s is not allowed to use flock')
+                              % fstype)
+        import fcntl # not available on all platforms / code paths
+        fcntl.flock(flockfd, fcntl.LOCK_NB | fcntl.LOCK_EX) # may raise EAGAIN
+        symname = '%s.tmp%d' % (pathname, os.getpid())
+    else:
+        openflags |= os.O_EXCL
+        symname = pathname
+
     try:
-        return os.symlink(info, pathname)
-    except OSError as why:
-        if why.errno == errno.EEXIST:
-            raise
-    except AttributeError: # no symlink in os
-        pass
-
-    ld = os.open(pathname, os.O_CREAT | os.O_WRONLY | os.O_EXCL)
-    os.write(ld, info)
-    os.close(ld)
+        try:
+            os.symlink(info, symname)
+            if symname != pathname:
+                os.rename(symname, pathname)
+            return
+        except OSError as why:
+            if why.errno == errno.EEXIST:
+                raise
+        except AttributeError: # no symlink in os
+            pass
+
+        ld = os.open(pathname, openflags)
+        os.write(ld, info)
+        os.close(ld)
+    except Exception:
+        if flockfd is not None:
+            fcntl.flock(flockfd, fcntl.LOCK_UN)
+        raise
 
 def readlock(pathname):
diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -147,6 +147,6 @@  class abstractvfs(object):
         return util.makedirs(self.join(path), mode)
 
-    def makelock(self, info, path):
-        return util.makelock(info, self.join(path))
+    def makelock(self, info, path, flockfd=None):
+        return util.makelock(info, self.join(path), flockfd)
 
     def mkdir(self, path=None):
diff --git a/tests/test-lock.py b/tests/test-lock.py
--- a/tests/test-lock.py
+++ b/tests/test-lock.py
@@ -33,5 +33,5 @@  class lockwrapper(lock.lock):
 
 class teststate(object):
-    def __init__(self, testcase, dir, pidoffset=0):
+    def __init__(self, testcase, dir, pidoffset=0, useflock=False):
         self._testcase = testcase
         self._acquirecalled = False
@@ -40,6 +40,9 @@  class teststate(object):
         self.vfs = vfsmod.vfs(dir, audit=False)
         self._pidoffset = pidoffset
+        self._useflock = useflock
 
     def makelock(self, *args, **kwargs):
+        if self._useflock:
+            kwargs['flockpath'] = self.vfs.base
         l = lockwrapper(self._pidoffset, self.vfs, testlockname,
                         releasefn=self.releasefn, acquirefn=self.acquirefn,
@@ -106,6 +109,9 @@  class teststate(object):
 
 class testlock(unittest.TestCase):
+    useflock = False
+
     def testlock(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -116,5 +122,6 @@  class testlock(unittest.TestCase):
 
     def testrecursivelock(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -136,5 +143,6 @@  class testlock(unittest.TestCase):
 
     def testlockfork(self):
-        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()))
+        state = teststate(self, tempfile.mkdtemp(dir=os.getcwd()),
+                          useflock=self.useflock)
         lock = state.makelock()
         state.assertacquirecalled(True)
@@ -156,5 +164,5 @@  class testlock(unittest.TestCase):
     def testinheritlock(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        parentstate = teststate(self, d)
+        parentstate = teststate(self, d, useflock=self.useflock)
         parentlock = parentstate.makelock()
         parentstate.assertacquirecalled(True)
@@ -166,5 +174,5 @@  class testlock(unittest.TestCase):
             parentstate.assertlockexists(True)
 
-            childstate = teststate(self, d, pidoffset=1)
+            childstate = teststate(self, d, pidoffset=1, useflock=self.useflock)
             childlock = childstate.makelock(parentlock=lockname)
             childstate.assertacquirecalled(True)
@@ -186,5 +194,5 @@  class testlock(unittest.TestCase):
     def testmultilock(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state0 = teststate(self, d)
+        state0 = teststate(self, d, useflock=self.useflock)
         lock0 = state0.makelock()
         state0.assertacquirecalled(True)
@@ -195,5 +203,5 @@  class testlock(unittest.TestCase):
             state0.assertlockexists(True)
 
-            state1 = teststate(self, d, pidoffset=1)
+            state1 = teststate(self, d, pidoffset=1, useflock=self.useflock)
             lock1 = state1.makelock(parentlock=lock0name)
             state1.assertacquirecalled(True)
@@ -205,5 +213,5 @@  class testlock(unittest.TestCase):
                 self.assertEqual(lock0name, lock1name)
 
-                state2 = teststate(self, d, pidoffset=2)
+                state2 = teststate(self, d, pidoffset=2, useflock=self.useflock)
                 lock2 = state2.makelock(parentlock=lock1name)
                 state2.assertacquirecalled(True)
@@ -227,5 +235,5 @@  class testlock(unittest.TestCase):
     def testinheritlockfork(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        parentstate = teststate(self, d)
+        parentstate = teststate(self, d, useflock=self.useflock)
         parentlock = parentstate.makelock()
         parentstate.assertacquirecalled(True)
@@ -233,5 +241,5 @@  class testlock(unittest.TestCase):
         # set up lock inheritance
         with parentlock.inherit() as lockname:
-            childstate = teststate(self, d, pidoffset=1)
+            childstate = teststate(self, d, pidoffset=1, useflock=self.useflock)
             childlock = childstate.makelock(parentlock=lockname)
             childstate.assertacquirecalled(True)
@@ -255,5 +263,5 @@  class testlock(unittest.TestCase):
     def testinheritcheck(self):
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state = teststate(self, d)
+        state = teststate(self, d, useflock=self.useflock)
         def check():
             raise error.LockInheritanceContractViolation('check failed')
@@ -275,5 +283,5 @@  class testlock(unittest.TestCase):
 
         d = tempfile.mkdtemp(dir=os.getcwd())
-        state = teststate(self, d)
+        state = teststate(self, d, useflock=self.useflock)
 
         def emulatefrequentlock(*args):
@@ -293,4 +301,7 @@  class testlock(unittest.TestCase):
             state.assertlockexists(False)
 
+class testflock(testlock):
+    useflock = True
+
 if __name__ == '__main__':
     silenttestrunner.main(__name__)