Patchwork repair: use context manager for lock management

login
register
mail settings
Submitter Matt Harbison
Date March 25, 2017, 3:34 a.m.
Message ID <c053dc8a24afad248723.1490412868@Envy>
Download mbox | patch
Permalink /patch/19654/
State Accepted
Headers show

Comments

Matt Harbison - March 25, 2017, 3:34 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1490327243 14400
#      Thu Mar 23 23:47:23 2017 -0400
# Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
# Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
repair: use context manager for lock management

If repo.lock() raised inside of the try block, 'tr' would have been None in the
finally block where it tries to release().  Modernize the syntax instead of just
winching the lock out of the try block.

I found several other instances of acquiring the lock inside of the 'try', but
those finally blocks handle None references.  I also started switching some
trivial try/finally blocks to context managers, but didn't get them all because
indenting over 3x for lock, wlock and transaction would have spilled over 80
characters.  That got me wondering if there should be a repo.rwlock(), to handle
locking and unlocking in the proper order.

It also looks like py27 supports supports multiple context managers for a single
'with' statement.  Should I hold off on the rest until py26 is dropped?
Yuya Nishihara - March 26, 2017, 11:20 a.m.
On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1490327243 14400
> #      Thu Mar 23 23:47:23 2017 -0400
> # Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
> # Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
> repair: use context manager for lock management

Sure. Queued this, thanks.

> I found several other instances of acquiring the lock inside of the 'try', but
> those finally blocks handle None references.  I also started switching some
> trivial try/finally blocks to context managers, but didn't get them all because
> indenting over 3x for lock, wlock and transaction would have spilled over 80
> characters.  That got me wondering if there should be a repo.rwlock(), to handle
> locking and unlocking in the proper order.

We have lockmod.release() helper. We also have util.ctxmanager(), but IMHO it
doesn't improve code readability that much.

> It also looks like py27 supports supports multiple context managers for a single
> 'with' statement.  Should I hold off on the rest until py26 is dropped?
Gregory Szorc - March 26, 2017, 5:34 p.m.
On Sun, Mar 26, 2017 at 4:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1490327243 14400
> > #      Thu Mar 23 23:47:23 2017 -0400
> > # Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
> > # Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
> > repair: use context manager for lock management
>
> Sure. Queued this, thanks.
>
> > I found several other instances of acquiring the lock inside of the
> 'try', but
> > those finally blocks handle None references.  I also started switching
> some
> > trivial try/finally blocks to context managers, but didn't get them all
> because
> > indenting over 3x for lock, wlock and transaction would have spilled
> over 80
> > characters.  That got me wondering if there should be a repo.rwlock(),
> to handle
> > locking and unlocking in the proper order.
>
> We have lockmod.release() helper. We also have util.ctxmanager(), but IMHO
> it
> doesn't improve code readability that much.
>
> > It also looks like py27 supports supports multiple context managers for
> a single
> > 'with' statement.  Should I hold off on the rest until py26 is dropped?
>

I think there is room for a helper context manager (like repo.rwlock())
that obtains multiple locks and/or a transaction. This would cut down on a
lot of excessive indentation while simultaneously ensuring we're doing the
correct thing with regards to locking and transaction semantics.
Matt Harbison - March 26, 2017, 7:28 p.m.
On Sun, 26 Mar 2017 13:34:10 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Sun, Mar 26, 2017 at 4:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>
>> On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
>> > # HG changeset patch
>> > # User Matt Harbison <matt_harbison@yahoo.com>
>> > # Date 1490327243 14400
>> > #      Thu Mar 23 23:47:23 2017 -0400
>> > # Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
>> > # Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
>> > repair: use context manager for lock management
>>
>> Sure. Queued this, thanks.
>>
>> > I found several other instances of acquiring the lock inside of the
>> 'try', but
>> > those finally blocks handle None references.  I also started switching
>> some
>> > trivial try/finally blocks to context managers, but didn't get them  
>> all
>> because
>> > indenting over 3x for lock, wlock and transaction would have spilled
>> over 80
>> > characters.  That got me wondering if there should be a repo.rwlock(),
>> to handle
>> > locking and unlocking in the proper order.
>>
>> We have lockmod.release() helper. We also have util.ctxmanager(), but  
>> IMHO
>> it
>> doesn't improve code readability that much.
>>
>> > It also looks like py27 supports supports multiple context managers  
>> for
>> a single
>> > 'with' statement.  Should I hold off on the rest until py26 is  
>> dropped?
>>
>
> I think there is room for a helper context manager (like repo.rwlock())
> that obtains multiple locks and/or a transaction. This would cut down on  
> a
> lot of excessive indentation while simultaneously ensuring we're doing  
> the
> correct thing with regards to locking and transaction semantics.

The excessive indentation is why I raised the issue.  I went looking for  
how to use util.ctxmanager, and can't find a single usage instance in the  
entire history.  I went back to the ML, and it seems like mpm ruled out  
using it [1].  The referenced context manager series he mentioned taking  
80% of seems to be something else [2].

I'm not sure if we want to revisit that decision, and even if so, I'm not  
sure if we want to start using a crutch for py26 if we are close to  
dumping it.  I only got off on this tangent when I noticed the  
first-lock-inside-try construct while trying to figure out bookmark  
pruning test failures on Windows.  So I don't have a strong opinion either  
way.

[1]  
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-January/078755.html
[2]  
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-January/078262.html
Yuya Nishihara - March 27, 2017, 12:53 p.m.
On Sun, 26 Mar 2017 15:28:02 -0400, Matt Harbison wrote:
> On Sun, 26 Mar 2017 13:34:10 -0400, Gregory Szorc  
> <gregory.szorc@gmail.com> wrote:
> > On Sun, Mar 26, 2017 at 4:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
> >> > # HG changeset patch
> >> > # User Matt Harbison <matt_harbison@yahoo.com>
> >> > # Date 1490327243 14400
> >> > #      Thu Mar 23 23:47:23 2017 -0400
> >> > # Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
> >> > # Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
> >> > repair: use context manager for lock management
> >>
> >> Sure. Queued this, thanks.
> >>
> >> > I found several other instances of acquiring the lock inside of the
> >> 'try', but
> >> > those finally blocks handle None references.  I also started switching
> >> some
> >> > trivial try/finally blocks to context managers, but didn't get them  
> >> all
> >> because
> >> > indenting over 3x for lock, wlock and transaction would have spilled
> >> over 80
> >> > characters.  That got me wondering if there should be a repo.rwlock(),
> >> to handle
> >> > locking and unlocking in the proper order.

Just nitpicking. The name "rwlock" would be misleading since both locks are
for writing.

> >> We have lockmod.release() helper. We also have util.ctxmanager(), but  
> >> IMHO
> >> it
> >> doesn't improve code readability that much.
> >>
> >> > It also looks like py27 supports supports multiple context managers  
> >> for
> >> a single
> >> > 'with' statement.  Should I hold off on the rest until py26 is  
> >> dropped?
> >>
> >
> > I think there is room for a helper context manager (like repo.rwlock())
> > that obtains multiple locks and/or a transaction. This would cut down on  
> > a
> > lot of excessive indentation while simultaneously ensuring we're doing  
> > the
> > correct thing with regards to locking and transaction semantics.
> 
> The excessive indentation is why I raised the issue.  I went looking for  
> how to use util.ctxmanager, and can't find a single usage instance in the  
> entire history.  I went back to the ML, and it seems like mpm ruled out  
> using it [1].  The referenced context manager series he mentioned taking  
> 80% of seems to be something else [2].

IIRC, that's because the scope of lock, wlock, txn, etc. is sometimes
different.

> I'm not sure if we want to revisit that decision, and even if so, I'm not  
> sure if we want to start using a crutch for py26 if we are close to  
> dumping it.  I only got off on this tangent when I noticed the  
> first-lock-inside-try construct while trying to figure out bookmark  
> pruning test failures on Windows.  So I don't have a strong opinion either  
> way.

I'm -1 for excessive indentation of large code block. I generally use 'with'
only when it improves readability.
Gregory Szorc - March 30, 2017, 2:16 a.m.
On Mon, Mar 27, 2017 at 5:53 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 26 Mar 2017 15:28:02 -0400, Matt Harbison wrote:
> > On Sun, 26 Mar 2017 13:34:10 -0400, Gregory Szorc
> > <gregory.szorc@gmail.com> wrote:
> > > On Sun, Mar 26, 2017 at 4:20 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > >> On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
> > >> > # HG changeset patch
> > >> > # User Matt Harbison <matt_harbison@yahoo.com>
> > >> > # Date 1490327243 14400
> > >> > #      Thu Mar 23 23:47:23 2017 -0400
> > >> > # Node ID c053dc8a24afad24872397e5cd3f57411fc7d172
> > >> > # Parent  d0c2db2d9f13dca534c598de050eb1919ef79059
> > >> > repair: use context manager for lock management
> > >>
> > >> Sure. Queued this, thanks.
> > >>
> > >> > I found several other instances of acquiring the lock inside of the
> > >> 'try', but
> > >> > those finally blocks handle None references.  I also started
> switching
> > >> some
> > >> > trivial try/finally blocks to context managers, but didn't get them
> > >> all
> > >> because
> > >> > indenting over 3x for lock, wlock and transaction would have spilled
> > >> over 80
> > >> > characters.  That got me wondering if there should be a
> repo.rwlock(),
> > >> to handle
> > >> > locking and unlocking in the proper order.
>
> Just nitpicking. The name "rwlock" would be misleading since both locks are
> for writing.
>

Derp. Naming things is hard.


>
> > >> We have lockmod.release() helper. We also have util.ctxmanager(), but
> > >> IMHO
> > >> it
> > >> doesn't improve code readability that much.
> > >>
> > >> > It also looks like py27 supports supports multiple context managers
> > >> for
> > >> a single
> > >> > 'with' statement.  Should I hold off on the rest until py26 is
> > >> dropped?
> > >>
> > >
> > > I think there is room for a helper context manager (like repo.rwlock())
> > > that obtains multiple locks and/or a transaction. This would cut down
> on
> > > a
> > > lot of excessive indentation while simultaneously ensuring we're doing
> > > the
> > > correct thing with regards to locking and transaction semantics.
> >
> > The excessive indentation is why I raised the issue.  I went looking for
> > how to use util.ctxmanager, and can't find a single usage instance in the
> > entire history.  I went back to the ML, and it seems like mpm ruled out
> > using it [1].  The referenced context manager series he mentioned taking
> > 80% of seems to be something else [2].
>
> IIRC, that's because the scope of lock, wlock, txn, etc. is sometimes
> different.
>
> > I'm not sure if we want to revisit that decision, and even if so, I'm not
> > sure if we want to start using a crutch for py26 if we are close to
> > dumping it.  I only got off on this tangent when I noticed the
> > first-lock-inside-try construct while trying to figure out bookmark
> > pruning test failures on Windows.  So I don't have a strong opinion
> either
> > way.
>
> I'm -1 for excessive indentation of large code block. I generally use
> 'with'
> only when it improves readability.
>

Python 2.7's multiple context managers syntax can be nice. But from my
experience, it is difficult to get multiple context managers on the same
line and the syntax looks weird across multiple lines, so I end up nesting
multiple context managers.

Also, for the case of locks and transactions, there is a specific
relationship between the context managers and the order things must be
performed in. To avoid coding errors that can occur with multiple context
managers (regardless of the syntax), I think having a "helper" context
manager that abstracts away the interaction between the lock(s) and
transactions could be useful.

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -214,15 +214,10 @@ 
 
         for m in updatebm:
             bm[m] = repo[newbmtarget].node()
-        lock = tr = None
-        try:
-            lock = repo.lock()
-            tr = repo.transaction('repair')
-            bm.recordchange(tr)
-            tr.close()
-        finally:
-            tr.release()
-            lock.release()
+
+        with repo.lock():
+            with repo.transaction('repair') as tr:
+                bm.recordchange(tr)
 
         # remove undo files
         for undovfs, undofile in repo.undofiles():