lock: avoid unintentional lock acquisition at failure of readlock stable
authorFUJIWARA Katsunori <foozy@lares.dti.ne.jp>
Mon, 01 May 2017 19:59:13 +0900
branchstable
changeset 32088 0d892d820a51
parent 32087 e1938d6051da
child 32089 f942dc80819f
lock: avoid unintentional lock acquisition at failure of readlock Acquiring lock by vfs.makelock() and getting lock holder (aka "locker") information by vfs.readlock() aren't atomic action. Therefore, failure of the former doesn't ensure success of the latter. Before this patch, lock is unintentionally acquired, if ENOENT causes failure of vfs.readlock() while 5 times retrying, because lock._trylock() returns to caller silently after retrying, and lock.lock() assumes that lock._trylock() returns successfully only if lock is acquired. In this case, lock symlink (or file) isn't created, even though lock is treated as acquired in memory. To avoid this issue, this patch makes lock._trylock() raise LockHeld(EAGAIN) at the end of it, if lock isn't acquired while retrying. An empty "locker" meaning "busy for frequent lock/unlock by many processes" might appear in an abortion message, if lock acquisition fails. Therefore, this patch also does: - use '%r' to increase visibility of "locker", even if it is empty - show hint message to explain what empty "locker" means
mercurial/lock.py
mercurial/scmutil.py
tests/test-lock.py
--- a/mercurial/lock.py	Mon May 01 19:58:52 2017 +0900
+++ b/mercurial/lock.py	Mon May 01 19:59:13 2017 +0900
@@ -151,6 +151,12 @@
                     raise error.LockUnavailable(why.errno, why.strerror,
                                                 why.filename, self.desc)
 
+        if not self.held:
+            # use empty locker to mean "busy for frequent lock/unlock
+            # by many processes"
+            raise error.LockHeld(errno.EAGAIN,
+                                 self.vfs.join(self.f), self.desc, "")
+
     def _readlock(self):
         """read lock and return its value
 
--- a/mercurial/scmutil.py	Mon May 01 19:58:52 2017 +0900
+++ b/mercurial/scmutil.py	Mon May 01 19:59:13 2017 +0900
@@ -151,10 +151,12 @@
     # Mercurial-specific first, followed by built-in and library exceptions
     except error.LockHeld as inst:
         if inst.errno == errno.ETIMEDOUT:
-            reason = _('timed out waiting for lock held by %s') % inst.locker
+            reason = _('timed out waiting for lock held by %r') % inst.locker
         else:
-            reason = _('lock held by %s') % inst.locker
+            reason = _('lock held by %r') % inst.locker
         ui.warn(_("abort: %s: %s\n") % (inst.desc or inst.filename, reason))
+        if not inst.locker:
+            ui.warn(_("(lock might be very busy)\n"))
     except error.LockUnavailable as inst:
         ui.warn(_("abort: could not lock %s: %s\n") %
                (inst.desc or inst.filename, inst.strerror))
--- a/tests/test-lock.py	Mon May 01 19:58:52 2017 +0900
+++ b/tests/test-lock.py	Mon May 01 19:59:13 2017 +0900
@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 
 import copy
+import errno
 import os
 import silenttestrunner
 import tempfile
@@ -267,5 +268,31 @@
 
         lock.release()
 
+    def testfrequentlockunlock(self):
+        """This tests whether lock acquisition fails as expected, even if
+        (1) lock can't be acquired (makelock fails by EEXIST), and
+        (2) locker info can't be read in (readlock fails by ENOENT) while
+        retrying 5 times.
+        """
+
+        d = tempfile.mkdtemp(dir=os.getcwd())
+        state = teststate(self, d)
+
+        def emulatefrequentlock(*args):
+            raise OSError(errno.EEXIST, "File exists")
+        def emulatefrequentunlock(*args):
+            raise OSError(errno.ENOENT, "No such file or directory")
+
+        state.vfs.makelock = emulatefrequentlock
+        state.vfs.readlock = emulatefrequentunlock
+
+        try:
+            state.makelock(timeout=0)
+            self.fail("unexpected lock acquisition")
+        except error.LockHeld as why:
+            self.assertTrue(why.errno == errno.ETIMEDOUT)
+            self.assertTrue(why.locker == "")
+            state.assertlockexists(False)
+
 if __name__ == '__main__':
     silenttestrunner.main(__name__)