| 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
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?
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.
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
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.
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():