[PATCH] repair: use context manager for lock management

[Yuya Nishihara]

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 at gmail.com> wrote:
> > On Sun, Mar 26, 2017 at 4:20 AM, Yuya Nishihara <yuya at tcha.org> wrote:
> >> On Fri, 24 Mar 2017 23:34:28 -0400, Matt Harbison wrote:
> >> > # HG changeset patch
> >> > # User Matt Harbison <matt_harbison at 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.