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

# 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

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

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.

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.