Patchwork [V2] lock: while releasing, unlink lockfile even if the release function throws

login
register
mail settings
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

Siddharth Agarwal - Oct. 17, 2014, 4:49 p.m.
# 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.
Pierre-Yves David - Oct. 17, 2014, 9:35 p.m.
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