changeset 23032:f484be02bd35

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.

--- a/mercurial/lock.py	Fri Oct 17 13:52:10 2014 -0400
+++ b/mercurial/lock.py	Thu Oct 16 19:15:51 2014 -0700
@@ -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()
 

--- a/tests/test-lock-badness.t	Fri Oct 17 13:52:10 2014 -0400
+++ b/tests/test-lock-badness.t	Thu Oct 16 19:15:51 2014 -0700
@@ -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