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

login
register
mail settings
Submitter Siddharth Agarwal
Date Oct. 17, 2014, 2:39 a.m.
Message ID <973c86a609dd4d30b351.1413513542@devbig136.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6359/
State Superseded
Headers show

Comments

Siddharth Agarwal - Oct. 17, 2014, 2:39 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1413512151 25200
#      Thu Oct 16 19:15:51 2014 -0700
# Node ID 973c86a609dd4d30b351bc5fa2210c380c7d04d1
# 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, 2:48 a.m.
On 10/16/2014 07:39 PM, 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 973c86a609dd4d30b351bc5fa2210c380c7d04d1
> # Parent  840be5ca03e1db16ba994e55597771c418166c97
> lock: while releasing, unlink lockfile even if the release function throws

The patch sound sensible, but I'll give it a second though in a couple 
of hours because it the area is sensitive.

Is it possible to produce an appropriate test for this (that does no 
deadlock when failing) so we do not regress this in the future?

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