[PATCH] repair: use context manager for lock management

Gregory Szorc gregory.szorc at gmail.com
Wed Mar 29 22:16:07 EDT 2017


On Mon, Mar 27, 2017 at 5:53 AM, Yuya Nishihara <yuya at 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 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.
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170329/eea85895/attachment.html>


More information about the Mercurial-devel mailing list