| 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
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__)