Submitter | Siddharth Agarwal |
---|---|
Date | Oct. 17, 2014, 4:49 p.m. |
Message ID | <5d07ba572ebef1d409e7.1413564588@devbig136.prn2.facebook.com> |
Download | mbox | patch |
Permalink | /patch/6370/ |
State | Accepted |
Headers | show |
Comments
On 10/17/2014 09:49 AM, Siddharth Agarwal wrote: > # HG changeset patch > # User Siddharth Agarwal <sid0@fb.com> > # Date 1413512151 25200 > # Thu Oct 16 19:15:51 2014 -0700 > # Node ID 5d07ba572ebef1d409e7dcc75f469248447cba7b > # Parent 840be5ca03e1db16ba994e55597771c418166c97 > lock: while releasing, unlink lockfile even if the release function throws > > Consider a hypothetical bug in the release function that causes it to raise an > exception. Also consider the bisect command, which saves its state in a finally > clause. Saving the state requires acquiring the wlock. > > If we don't unlink the lockfile when the exception is thrown, we'll try to > acquire the wlock again. We're going to try and acquire a lock again while our > old lockfile is on disk. The PID on disk is our own, and of course we're still > running, so we won't take over the lock. Hence we'll be stuck waiting for a > lock that we left behind ourselves. > > To avoid this, always unlink the lockfile. This preserves the invariant that > self.held > 0 is equivalent to the lockfile existing on disk. This version looks fine to me and have been pushed to the clowncopter.
Patch
diff --git a/mercurial/lock.py b/mercurial/lock.py --- a/mercurial/lock.py +++ b/mercurial/lock.py @@ -139,12 +139,14 @@ if os.getpid() != self.pid: # we forked, and are not the parent return - if self.releasefn: - self.releasefn() try: - self.vfs.unlink(self.f) - except OSError: - pass + if self.releasefn: + self.releasefn() + finally: + try: + self.vfs.unlink(self.f) + except OSError: + pass for callback in self.postrelease: callback() diff --git a/tests/test-lock-badness.t b/tests/test-lock-badness.t --- a/tests/test-lock-badness.t +++ b/tests/test-lock-badness.t @@ -11,6 +11,42 @@ updating to branch default 1 files updated, 0 files merged, 0 files removed, 0 files unresolved +Test that raising an exception in the release function doesn't cause the lock to choke + + $ cat > testlock.py << EOF + > from mercurial import cmdutil, error, util + > + > cmdtable = {} + > command = cmdutil.command(cmdtable) + > + > def acquiretestlock(repo, releaseexc): + > def unlock(): + > if releaseexc: + > raise util.Abort('expected release exception') + > l = repo._lock(repo.vfs, 'testlock', False, unlock, None, 'test lock') + > return l + > + > @command('testlockexc') + > def testlockexc(ui, repo): + > testlock = acquiretestlock(repo, True) + > try: + > testlock.release() + > finally: + > try: + > testlock = acquiretestlock(repo, False) + > except error.LockHeld: + > raise util.Abort('lockfile on disk even after releasing!') + > testlock.release() + > EOF + $ cat >> $HGRCPATH << EOF + > [extensions] + > testlock=$TESTTMP/testlock.py + > EOF + + $ hg -R b testlockexc + abort: expected release exception + [255] + One process waiting for another $ cat > hooks.py << EOF