Patchwork [2,of,4,STABLE,V3] lock: avoid unintentional lock acquisition at failure of readlock

login
register
mail settings
Submitter Katsunori FUJIWARA
Date May 1, 2017, 11:02 a.m.
Message ID <cc349553a2126c8e28bc.1493636570@juju>
Download mbox | patch
Permalink /patch/20327/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - May 1, 2017, 11:02 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1493636353 -32400
#      Mon May 01 19:59:13 2017 +0900
# Branch stable
# Node ID cc349553a2126c8e28bc24402bab3bad7981c8c0
# Parent  e1938d6051da1eff0c0d78fc7aae0d426e99aad2
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 ENOEXIST
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
Yuya Nishihara - May 1, 2017, 1:23 p.m.
On Mon, 01 May 2017 20:02:50 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1493636353 -32400
> #      Mon May 01 19:59:13 2017 +0900
> # Branch stable
> # Node ID cc349553a2126c8e28bc24402bab3bad7981c8c0
> # Parent  e1938d6051da1eff0c0d78fc7aae0d426e99aad2
> 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 ENOEXIST
> 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
> 
> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -151,6 +151,12 @@ class lock(object):
>                      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, "")

For reference, this bug would probably exist since 3b6e5914edd8 "lock: loop
a finite number of times in trylock (issue4787)." Before, trylock() would
never return until lock was taken.

Patch

diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -151,6 +151,12 @@  class lock(object):
                     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
 
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -151,10 +151,12 @@  def callcatch(ui, func):
     # 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))
diff --git a/tests/test-lock.py b/tests/test-lock.py
--- a/tests/test-lock.py
+++ b/tests/test-lock.py
@@ -1,6 +1,7 @@ 
 from __future__ import absolute_import
 
 import copy
+import errno
 import os
 import silenttestrunner
 import tempfile
@@ -267,5 +268,31 @@  class testlock(unittest.TestCase):
 
         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__)