Patchwork [3,of,6] amend: fix unlocking order - first lock then wlock

login
register
mail settings
Submitter Mads Kiilerich
Date April 17, 2013, 2:11 a.m.
Message ID <50b42260018a4739637c.1366164663@xps>
Download mbox | patch
Permalink /patch/1371/
State Accepted
Commit ab04e87a5f3bcee588b72d615e1aeb42f10d3b99
Headers show

Comments

Mads Kiilerich - April 17, 2013, 2:11 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1366162868 -7200
# Node ID 50b42260018a4739637cde911f42c04ecd402c88
# Parent  815cbc05cd06440e71448fd43d20142d2fc1db91
amend: fix unlocking order - first lock then wlock
Augie Fackler - April 17, 2013, 3:14 a.m.
queued the first three of this series, 4 needs a resend (momentarily), don't feel qualified to review 5.

On Apr 16, 2013, at 10:11 PM, Mads Kiilerich <mads@kiilerich.com> wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1366162868 -7200
> # Node ID 50b42260018a4739637cde911f42c04ecd402c88
> # Parent  815cbc05cd06440e71448fd43d20142d2fc1db91
> amend: fix unlocking order - first lock then wlock
> 
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -1790,7 +1790,7 @@ def amend(ui, repo, commitfunc, old, ext
>     finally:
>         if newid is None:
>             repo.dirstate.invalidate()
> -        lockmod.release(wlock, lock)
> +        lockmod.release(lock, wlock)
>     return newid
> 
> def commiteditor(repo, ctx, subs):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - April 17, 2013, 6:59 p.m.
On Wed, 2013-04-17 at 10:28 -0500, Kevin Bullock wrote:
> On Apr 16, 2013, at 9:11 PM, Mads Kiilerich wrote:
> 
> > # HG changeset patch
> > # User Mads Kiilerich <madski@unity3d.com>
> > # Date 1366162868 -7200
> > # Node ID 50b42260018a4739637cde911f42c04ecd402c88
> > # Parent  815cbc05cd06440e71448fd43d20142d2fc1db91
> > amend: fix unlocking order - first lock then wlock
> > 
> > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> > --- a/mercurial/cmdutil.py
> > +++ b/mercurial/cmdutil.py
> > @@ -1790,7 +1790,7 @@ def amend(ui, repo, commitfunc, old, ext
> >     finally:
> >         if newid is None:
> >             repo.dirstate.invalidate()
> > -        lockmod.release(wlock, lock)
> > +        lockmod.release(lock, wlock)
> 
> This contradicts the example code on <http://mercurial.selenic.com/wiki/LockingDesign>.

Yes, this change is incorrect. Locks should always be acquired in a
well-defined order and released in the opposite order as a matter of
good practice. Otherwise you risk deadlock.

As a practical matter, there's no risk here, since we're going to
release both locks immediately. But it's possible we might currently or
in the future rely on the rule for correctness and/or ordering in our
release callbacks.

I'll fix 'em up.
Matt Mackall - April 17, 2013, 7:33 p.m.
On Wed, 2013-04-17 at 13:59 -0500, Matt Mackall wrote:
> On Wed, 2013-04-17 at 10:28 -0500, Kevin Bullock wrote:
> > On Apr 16, 2013, at 9:11 PM, Mads Kiilerich wrote:
> > 
> > > # HG changeset patch
> > > # User Mads Kiilerich <madski@unity3d.com>
> > > # Date 1366162868 -7200
> > > # Node ID 50b42260018a4739637cde911f42c04ecd402c88
> > > # Parent  815cbc05cd06440e71448fd43d20142d2fc1db91
> > > amend: fix unlocking order - first lock then wlock
> > > 
> > > diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> > > --- a/mercurial/cmdutil.py
> > > +++ b/mercurial/cmdutil.py
> > > @@ -1790,7 +1790,7 @@ def amend(ui, repo, commitfunc, old, ext
> > >     finally:
> > >         if newid is None:
> > >             repo.dirstate.invalidate()
> > > -        lockmod.release(wlock, lock)
> > > +        lockmod.release(lock, wlock)
> > 
> > This contradicts the example code on <http://mercurial.selenic.com/wiki/LockingDesign>.
> 
> Yes, this change is incorrect. Locks should always be acquired in a
> well-defined order and released in the opposite order as a matter of
> good practice. Otherwise you risk deadlock.
> 
> As a practical matter, there's no risk here, since we're going to
> release both locks immediately. But it's possible we might currently or
> in the future rely on the rule for correctness and/or ordering in our
> release callbacks.
> 
> I'll fix 'em up.

Derp. The wiki is backwards. This is the locking order:

try:
 wlock =
 lock =
 tr =
finally:
 release(tr, lock, wlock)

The most important rule is wlock-taken-before-lock. I'll fix the wiki.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1790,7 +1790,7 @@  def amend(ui, repo, commitfunc, old, ext
     finally:
         if newid is None:
             repo.dirstate.invalidate()
-        lockmod.release(wlock, lock)
+        lockmod.release(lock, wlock)
     return newid
 
 def commiteditor(repo, ctx, subs):