Patchwork {RFC} commands: switch to ctxmanager in phase

login
register
mail settings
Submitter Bryan O'Sullivan
Date Jan. 11, 2016, 11:34 p.m.
Message ID <e909e11e2d42cf419f22.1452555272@bryano-mbp.local>
Download mbox | patch
Permalink /patch/12665/
State Not Applicable
Headers show

Comments

Bryan O'Sullivan - Jan. 11, 2016, 11:34 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bos@serpentine.com>
# Date 1452555256 28800
#      Mon Jan 11 15:34:16 2016 -0800
# Node ID e909e11e2d42cf419f2205e632f23a206a66d2f1
# Parent  03e3c89b14b24b99e63ec985e71d9ddd0ef7fcf1
{RFC} commands: switch to ctxmanager in phase

This is a small example of the kind of simplification that the ctxmanager class in the previous patch makes possible.
Bryan O'Sullivan - Jan. 11, 2016, 11:40 p.m.
On Mon, Jan 11, 2016 at 3:34 PM, Bryan O'Sullivan <bos@serpentine.com>
wrote:

> +        with util.ctxmanager(repo.lock, repo.translambda('phase')) as c:
>

I forgot the patch that introduces translambda; it simply defers
repo.transaction with a lambda because that pattern is used so heavily.

This patch series is several dozen long, all very simple transformations of
this sort. I think the introductory patch should be enough to get
conversation started :-)
Gregory Szorc - Jan. 12, 2016, 12:11 a.m.
On Mon, Jan 11, 2016 at 3:40 PM, Bryan O'Sullivan <bos@serpentine.com>
wrote:

>
> On Mon, Jan 11, 2016 at 3:34 PM, Bryan O'Sullivan <bos@serpentine.com>
> wrote:
>
>> +        with util.ctxmanager(repo.lock, repo.translambda('phase')) as c:
>>
>
> I forgot the patch that introduces translambda; it simply defers
> repo.transaction with a lambda because that pattern is used so heavily.
>
> This patch series is several dozen long, all very simple transformations
> of this sort. I think the introductory patch should be enough to get
> conversation started :-)
>

+1 to moving locks and transactions to context managers.

As much as we use the pattern of obtaining a lock *and* a transaction, I
think it might be worthwhile to introduce a utility context manager. e.g.

with repo.lockandtransaction('phase') as c:
    lock, tr = c
Yuya Nishihara - Jan. 16, 2016, 12:15 p.m.
A couple of issues found in "use context manager" series, in clowncopter.

http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292

> --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> @@ -1548,17 +1548,15 @@
>                  # so we don't wait on the lock
>                  # wlock can invalidate the dirstate, so cache normal _after_
>                  # taking the lock
> -                wlock = self._repo.wlock(False)
>                  normal = self._repo.dirstate.normal
> -                try:
> +                with self._repo.wlock(False):

wlock must be taken before caching normal. See
https://selenic.com/repo/hg/rev/48e32c2c499b


http://hg.netv6.net/clowncopter/rev/adf95f8332e6

> --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> @@ -1415,9 +1415,8 @@
>  
>      def add(self, list, prefix=""):
>          join = lambda f: os.path.join(prefix, f)
> -        wlock = self._repo.wlock()
>          ui, ds = self._repo.ui, self._repo.dirstate
> -        try:
> +        with self._repo.wlock():

Perhaps dirstate should be cached after taking wlock.


http://hg.netv6.net/clowncopter/rev/5eaee365f782

> @@ -258,14 +257,12 @@
>  
>  class histeditstate(object):
>      def __init__(self, repo, parentctxnode=None, actions=None, keep=None,
> -            topmost=None, replacements=None, lock=None, wlock=None):
> +            topmost=None, replacements=None):
>          self.repo = repo
>          self.actions = actions
>          self.keep = keep
>          self.topmost = topmost
>          self.parentctxnode = parentctxnode
> -        self.lock = lock
> -        self.wlock = wlock

These variables are planned to be used?

https://selenic.com/repo/hg/rev/e0b5f5e3afe8

Regards,
Pierre-Yves David - Jan. 17, 2016, 4:25 a.m.
On 01/16/2016 04:15 AM, Yuya Nishihara wrote:
> A couple of issues found in "use context manager" series, in clowncopter.
>
> http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292
>
>> --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
>> +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
>> @@ -1548,17 +1548,15 @@
>>                   # so we don't wait on the lock
>>                   # wlock can invalidate the dirstate, so cache normal _after_
>>                   # taking the lock
>> -                wlock = self._repo.wlock(False)
>>                   normal = self._repo.dirstate.normal
>> -                try:
>> +                with self._repo.wlock(False):
>
> wlock must be taken before caching normal. See
> https://selenic.com/repo/hg/rev/48e32c2c499b

Nice catch, I should have spotted that. I'm not sure what the status of 
that series in Matt hands. We sould send a fix once that status is clearer.


> http://hg.netv6.net/clowncopter/rev/adf95f8332e6
>
>> --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
>> +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
>> @@ -1415,9 +1415,8 @@
>>
>>       def add(self, list, prefix=""):
>>           join = lambda f: os.path.join(prefix, f)
>> -        wlock = self._repo.wlock()
>>           ui, ds = self._repo.ui, self._repo.dirstate
>> -        try:
>> +        with self._repo.wlock():
>
> Perhaps dirstate should be cached after taking wlock.

ditto.


>
>
> http://hg.netv6.net/clowncopter/rev/5eaee365f782
>
>> @@ -258,14 +257,12 @@
>>
>>   class histeditstate(object):
>>       def __init__(self, repo, parentctxnode=None, actions=None, keep=None,
>> -            topmost=None, replacements=None, lock=None, wlock=None):
>> +            topmost=None, replacements=None):
>>           self.repo = repo
>>           self.actions = actions
>>           self.keep = keep
>>           self.topmost = topmost
>>           self.parentctxnode = parentctxnode
>> -        self.lock = lock
>> -        self.wlock = wlock
>
> These variables are planned to be used?
>
> https://selenic.com/repo/hg/rev/e0b5f5e3afe8

Without a comment explaining why we have these unused variable, there is 
few chance that they survive. We should reinstall them with a such comment.

Cheers,
Matt Mackall - Jan. 17, 2016, 1:39 p.m.
On Sat, 2016-01-16 at 20:25 -0800, Pierre-Yves David wrote:
> 
> On 01/16/2016 04:15 AM, Yuya Nishihara wrote:
> > A couple of issues found in "use context manager" series, in clowncopter.
> > 
> > http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292
> > 
> > > --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > @@ -1548,17 +1548,15 @@
> > >                   # so we don't wait on the lock
> > >                   # wlock can invalidate the dirstate, so cache normal
> > > _after_
> > >                   # taking the lock
> > > -                wlock = self._repo.wlock(False)
> > >                   normal = self._repo.dirstate.normal
> > > -                try:
> > > +                with self._repo.wlock(False):
> > 
> > wlock must be taken before caching normal. See
> > https://selenic.com/repo/hg/rev/48e32c2c499b
> 
> Nice catch, I should have spotted that. I'm not sure what the status of 
> that series in Matt hands. We sould send a fix once that status is clearer.

I've got a bunch of other nits in my review notes for this series. There's also
some stuff of the form:

-        finally:
-            tr.release()
-            repo.ui.flush()
+        repo.ui.flush()

..that reverts a bugfix. We don't flush ui buffers much, so doing it in a
finally clause is a good sign that there's an issue being dealt with.

I'm going to try to sort through it today.

> 
> > http://hg.netv6.net/clowncopter/rev/adf95f8332e6
> > 
> > > --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > @@ -1415,9 +1415,8 @@
> > > 
> > >       def add(self, list, prefix=""):
> > >           join = lambda f: os.path.join(prefix, f)
> > > -        wlock = self._repo.wlock()
> > >           ui, ds = self._repo.ui, self._repo.dirstate
> > > -        try:
> > > +        with self._repo.wlock():
> > 
> > Perhaps dirstate should be cached after taking wlock.
> 
> ditto.
> 
> 
> > 
> > 
> > http://hg.netv6.net/clowncopter/rev/5eaee365f782
> > 
> > > @@ -258,14 +257,12 @@
> > > 
> > >   class histeditstate(object):
> > >       def __init__(self, repo, parentctxnode=None, actions=None,
> > > keep=None,
> > > -            topmost=None, replacements=None, lock=None, wlock=None):
> > > +            topmost=None, replacements=None):
> > >           self.repo = repo
> > >           self.actions = actions
> > >           self.keep = keep
> > >           self.topmost = topmost
> > >           self.parentctxnode = parentctxnode
> > > -        self.lock = lock
> > > -        self.wlock = wlock
> > 
> > These variables are planned to be used?
> > 
> > https://selenic.com/repo/hg/rev/e0b5f5e3afe8
> 
> Without a comment explaining why we have these unused variable, there is 
> few chance that they survive. We should reinstall them with a such comment.
> 
> Cheers,
>
Bryan O'Sullivan - Jan. 22, 2016, 11 p.m.
On Sun, Jan 17, 2016 at 5:39 AM, Matt Mackall <mpm@selenic.com> wrote:

> I'm going to try to sort through it today.
>

Got more details?
Matt Mackall - Jan. 22, 2016, 11:18 p.m.
On Sun, 2016-01-17 at 07:39 -0600, Matt Mackall wrote:
> On Sat, 2016-01-16 at 20:25 -0800, Pierre-Yves David wrote:
> > 
> > On 01/16/2016 04:15 AM, Yuya Nishihara wrote:
> > > A couple of issues found in "use context manager" series, in clowncopter.
> > > 
> > > http://hg.netv6.net/clowncopter/rev/5f2bd1d7b292
> > > 
> > > > --- a/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > > +++ b/mercurial/context.py	Fri Jan 15 13:14:46 2016 -0800
> > > > @@ -1548,17 +1548,15 @@
> > > >                   # so we don't wait on the lock
> > > >                   # wlock can invalidate the dirstate, so cache normal
> > > > _after_
> > > >                   # taking the lock
> > > > -                wlock = self._repo.wlock(False)
> > > >                   normal = self._repo.dirstate.normal
> > > > -                try:
> > > > +                with self._repo.wlock(False):
> > > 
> > > wlock must be taken before caching normal. See
> > > https://selenic.com/repo/hg/rev/48e32c2c499b
> > 
> > Nice catch, I should have spotted that. I'm not sure what the status of 
> > that series in Matt hands. We sould send a fix once that status is clearer.
> 
> I've got a bunch of other nits in my review notes for this series. There's
> also
> some stuff of the form:
> 
> -        finally:
> -            tr.release()
> -            repo.ui.flush()
> +        repo.ui.flush()
> 
> ..that reverts a bugfix. We don't flush ui buffers much, so doing it in a
> finally clause is a good sign that there's an issue being dealt with.
> 
> I'm going to try to sort through it today.

For the record, I ended up taking about 80% of this series. I did the following:

- reordered the whole series
  - cleanups first
  - context manager definitions
  - single locks
  - just transactions
  - lock+wlock
  - lock+tranaction
- changed the summary keywords of everything to "with:"
  (the keywords mostly exist to sort our end-user release notes)
- audited carefully for entry and exit semantic changes like the above

I ended up not taking the multi-element changes because I have several concerns
and we were up against the freeze. 

It seems to me that in general, trying to collapse multiple resource
acquisitions into one "with:" is error-prone and doesn't always work because we
often want a finally: or except: that's orthogonal to resource acquisition.
We even have stuff we want to do between acquisition of resources and may not
always want to acquire things early.

So it seems that the right thing is to just consistently use one with: for each
resource and accept the extra level(s) of indentation as the cost of clarity and
consistency (and curse Python for having useless destructors).

We're also going to in the not to distant future figure out how to break up our
locks a bit more, so the patterns for lockall and lockedtxn are a bit doomed
anyway.

Lastly, it's rather unfortunate that Python's "context manager" name collides
with our own very different and heavily used notion of context. We should try
and minimize that collision, starting with not naming anything with "ctx" which
is the abbreviation our context classes and variables all use (bearing in mind
that naming consistency is one of our main forms of type safety lolfml).

-- 
Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -5438,7 +5438,6 @@  def phase(ui, repo, *revs, **opts):
 
     revs = scmutil.revrange(repo, revs)
 
-    lock = None
     ret = 0
     if targetphase is None:
         # display
@@ -5446,10 +5445,8 @@  def phase(ui, repo, *revs, **opts):
             ctx = repo[r]
             ui.write('%i: %s\n' % (ctx.rev(), ctx.phasestr()))
     else:
-        tr = None
-        lock = repo.lock()
-        try:
-            tr = repo.transaction("phase")
+        with util.ctxmanager(repo.lock, repo.translambda('phase')) as c:
+            tr = c()[1]
             # set phase
             if not revs:
                 raise error.Abort(_('empty revision set'))
@@ -5463,10 +5460,6 @@  def phase(ui, repo, *revs, **opts):
             if opts['force']:
                 phases.retractboundary(repo, tr, targetphase, nodes)
             tr.close()
-        finally:
-            if tr is not None:
-                tr.release()
-            lock.release()
         getphase = unfi._phasecache.phase
         newdata = [getphase(unfi, r) for r in unfi]
         changes = sum(newdata[r] != olddata[r] for r in unfi)