[PATCH STABLE] extensions: clear aftercallbacks after execution (issue4646)

[Gregory Szorc]

On Tue, May 5, 2015 at 11:51 PM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> On 05/05/2015 10:39 PM, Yuya Nishihara wrote:
>
>> On Tue, 05 May 2015 21:35:25 -0700, Gregory Szorc wrote:
>>
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1430886916 25200
>>> #      Tue May 05 21:35:16 2015 -0700
>>> # Branch stable
>>> # Node ID 4da7e74074010404744171ad68c1a1bd7d100696
>>> # Parent  41cd8171e58f991373dcd0b4897dc1e5978d42dd
>>> extensions: clear aftercallbacks after execution (issue4646)
>>>
>>> It was reported that enabling pager without color could cause a hang.
>>> Inserting print statements revealed that the callbacks in
>>> extensions._aftercallbacks were being invoked twice.
>>>
>>> I'm not sure exactly how the hang comes to be. But, clearing
>>> extensions._aftercallbacks makes the problem go away. This presumably
>>> has something to do with forked processes inheriting
>>> extensions._aftercallbacks and that somehow results in badness.
>>>
>>> The reproduce steps in the bug seem to only occur when the output of
>>> a command is less than the size of the current screen. This is not
>>> something that can easily be tested. I verified the test case works
>>> with this patch and that pager and color interaction continues to
>>> work. Since we have no existing automated tests for pager, this sadly
>>> appears to be the best I can do.
>>>
>>> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
>>> --- a/mercurial/extensions.py
>>> +++ b/mercurial/extensions.py
>>> @@ -133,8 +133,12 @@ def loadall(ui):
>>>
>>>           for fn in _aftercallbacks[shortname]:
>>>               fn(loaded=False)
>>>
>>> +    # Forked processes appear to inherit _aftercallbacks, which may
>>> cause
>>> +    # deadlock. See issue4646.
>>> +    _aftercallbacks.clear()
>>>
>>
>> I don't think this is related to fork(). extensions.loadall() is just
>> called
>> more than once.
>>
>> % grep extensions.loadall mercurial/*.py
>> mercurial/dispatch.py:    extensions.loadall(lui)
>> mercurial/dispatch.py:    # (uisetup and extsetup are handled in
>> extensions.loadall)
>> mercurial/localrepo.py:            extensions.loadall(self.ui)
>>
>
> We probably call it (1) at the very beginning to load global/user enabled
> extension (that could be defining new command or alter the repo creation
> process). (2) after a repo is created and its UI is parsed.


Makes sense to me. (I wrote the patch after a few beers.)

I'll send a v2 with updated comments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150506/064f23b2/attachment.html>