Mailing-List: contact bloodhound-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: bloodhound-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of gary.martin@wandisco.com
 designates 74.125.83.47 as permitted sender)
Message-ID: <4F7C7CA2.5020907@wandisco.com>
Date: Wed, 04 Apr 2012 17:53:54 +0100
From: Gary <gary.martin@wandisco.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:11.0) Gecko/20120310 Thunderbird/11.0
MIME-Version: 1.0
To: bloodhound-dev@incubator.apache.org
Subject: Re: [Apache Bloodhound] #22: Infinite recursion error when
 initialising
 a plugin that extends itself
References: <052.4da5099d4a3aefbb8748f007a5dcd0fc@incubator.apache.org>
 <067.375d46ae7cfafe0033257c91c1f333a3@incubator.apache.org>
 <CAMmgw9=_+B14ciKPH68AbGhnDG8zFSHsQ5b-rxNBa1ssgKG2ug@mail.gmail.com>
In-Reply-To: 
 <CAMmgw9=_+B14ciKPH68AbGhnDG8zFSHsQ5b-rxNBa1ssgKG2ug@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 04/04/2012 05:43 PM, Ethan Jucovy wrote:
>>   [
>> http://svn.apache.org/viewvc/incubator/bloodhound/trunk/trac/trac/core.py?view=diff&r1=1309469&r2=1309470&pathrev=1309470
>>   r1309470] fixes by dynamically replacing the __init__ of components with
>>   one that runs the original __init__ on the condition that it has not
>>   already been run.
>>
>>   I suspect what would be better in the long run would be to catch the
>>   recursion error and turn it into something that can be reported but allows
>>   Bloodhound to continue working. Plugins should probably keep track of
>>   whether an instance has already been initialised.
>
> This issue was raised on trac-dev[1].  It was pointed out that the
> ThemeEngine plugin is incorrectly accessing its extension points in
> __init__, which it should not do:
>
> "A component constructor is called in a
> special way, as you've found out, and it should really do pretty
> much nothing.  Example of valid use is to set a list to [], a
> hash to {}, or such.  In trunk you could even use @lazy
> (http://trac.edgewall.org/changeset/10737) to help with such
> things. So doing anything lengthy is prohibited and the same
> for "re-entering" the component machinery."
>
> Perhaps Trac core should be throwing a more helpful error than infinite
> recursion, but the correct fix here is to make ThemeEngineSystem.__init__
> do less (e.g. ``self._info = None``) and populate its ``self.info`` dict
> lazily.  There's no need for this __init__-replacing magic in core.
>
> [1]
> http://old.nabble.com/Create-new-ticket-vs-reopen--9418-(if-necessary--)-td33164546.html
>

Ah, thanks for pointing that out. I was about to write a ticket to get 
someone to get the theme engine to change the implementation.

I don't particularly mind that a plugin wants to do a lot in their 
__init__ as long as they are prepared to deal with the potential error. 
As I think I already say, the only fault I see in Trac on this issue is 
that it doesn't deal with it quite helpfully enough. Basically I think 
it should survive the event to the extent of allowing the admin to 
disable the offending plugin from the admin interface.

Cheers,
     Gary