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

login
register
mail settings
Submitter Gregory Szorc
Date May 6, 2015, 4:35 a.m.
Message ID <4da7e740740104047441.1430886925@gps-mbp.local>
Download mbox | patch
Permalink /patch/8930/
State Superseded
Commit e6e7d1cce04da4ddff9d89794101eabb82640ede
Headers show

Comments

Gregory Szorc - May 6, 2015, 4:35 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@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.
Yuya Nishihara - May 6, 2015, 5:39 a.m.
On Tue, 05 May 2015 21:35:25 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@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)
Pierre-Yves David - May 6, 2015, 6:51 a.m.
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@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.
Gregory Szorc - May 6, 2015, 4:51 p.m.
On Tue, May 5, 2015 at 11:51 PM, Pierre-Yves David <
pierre-yves.david@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@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.

Patch

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()
+
 def afterloaded(extension, callback):
     '''Run the specified function after a named extension is loaded.
 
     If the named extension is already loaded, the callback will be called