Modify

Opened 3 years ago

Closed 2 years ago

#9580 closed defect (fixed)

Recursion after installing in Trac 0.13 (dev)

Reported by: gary.martin@… Owned by: olemis
Priority: normal Component: ThemeEnginePlugin
Severity: normal Keywords:
Cc: rjollos Trac Release: 0.12

Description

A change in the component system for 0.13 appears to have broken the ThemeEnginePlugin such that ThemeEngineSystem.__init__() is called recursively. The attached patch stops the ThemeEngineSystem class from implementing IThemeProvider and separates out the code relating to the default theme into its own class.

Attachments (1)

theme-engine-system-recursion.diff (4.3 KB) - added by gary.martin@… 3 years ago.
patch for recursion error

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by gary.martin@…

patch for recursion error

comment:1 Changed 3 years ago by olemis

Recently I tried to install this plugin in 0.13dev and got similar results i.e. it fails at load time . Fact is that this happens due to the changes committed in 0.12 (more precisely trac:changeset:10298 and trac:changeset:10300 . The whole story is explained in trac:ticket:9418 ;)

I'll be reporting this in t.e.o. issue tracker . I suggest to close this ticket as wontfix .

comment:2 Changed 3 years ago by anonymous

The comment above is not accurate. Patch attached to trac:ticket:9418 was committed in trac:changeset:10295 . Revisions mentioned above add further changes on top of it .

comment:3 Changed 3 years ago by olemis

  • Resolution set to wontfix
  • Status changed from new to closed

As long as there's no definitive solution provided by trac-dev , users experiencing this issue can try to apply this patch to Trac distribution .

https://bitbucket.org/olemis/trac-mq/raw/tho_themeengine/tho/themeengine/t-9580-infinite-recursion.diff

This bug is definitely caused by Trac core rather than the plugin , hence I'm closing the ticket with wontfix (... at least for the moment ;)

comment:4 Changed 3 years ago by anonymous

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Reopening ticket as this won't be fixed neither in 0.13 nor even later :-/

comment:5 follow-up: Changed 2 years ago by anonymous

    def __init__(self):
        self._info = None

    @property
    def info(self):
        # This can safely be cached because the data can only change on a restart anyway
        if self._info is None:
            self._info = {}
            for provider in self.providers:
                for name in provider.get_theme_names():
                    theme = provider.get_theme_info(name)
                    theme['provider'] = provider
                    theme['module'] = provider.__class__.__module__
                    theme['name'] = name
                    self._info[name.lower()] = theme
        return self._info

comment:6 Changed 2 years ago by rjollos

  • Cc rjollos added; anonymous removed

comment:7 in reply to: ↑ 5 Changed 2 years ago by gary.martin@…

Replying to anonymous

That looks like a really nice solution to me. The idea of deferring calculation of ThemeEngineSystem.info seems sound and a few quick tests suggest to me that it should work.

comment:8 Changed 2 years ago by anonymous

As suggested by cboos you could use @lazy since Trac 1.0 (or by copying it):

from trac.util import lazy
...
    @lazy 
    def info(self):
        result = {}
        for provider in self.providers:
            for name in provider.get_theme_names():
                theme = provider.get_theme_info(name)
                theme['provider'] = provider
                theme['module'] = provider.__class__.__module__
                theme['name'] = name
                result[name.lower()] = theme
        return result

comment:9 follow-up: Changed 2 years ago by olemis

  • Owner changed from coderanger to olemis
  • Status changed from reopened to new

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

  • trunk/setup.py

     
    1414
    1515setup(
    1616    name = 'TracThemeEngine',
    17     version = '2.0.1',
     17    version = '2.1.0',
    1818    packages = ['themeengine'],
    1919    package_data = { 'themeengine': ['templates/*.html', 'htdocs/*.js', 'htdocs/*.css', 'htdocs/img/*.gif',
    2020                                     'htdocs/farbtastic/*.png', 'htdocs/farbtastic/*.js', 'htdocs/farbtastic/*.css' ] },
  • trunk/themeengine/api.py

     
    1313from trac.core import *
    1414from trac.config import Option
    1515
     16try:
     17    from trac.util import lazy
     18except ImportError:
     19    lazy = None
     20
    1621class ThemeNotFound(TracError):
    1722    """The requested theme isn't found."""
    1823   
     
    6772    providers = ExtensionPoint(IThemeProvider)
    6873   
    6974    def __init__(self):
    70         # This can safely go in here because the data can only change on a restart anyway
    71         self.info = {}
     75        if lazy is None:
     76            # Trac < 0.12 : this can safely go in here because the data can
     77            # only change on a restart anyway
     78            self.info = self.info()
     79
     80    def info(self):
     81        # Trac >= 0.12 : Hack needed to deal with infinite recusrion error
     82        #    Details : http://trac-hacks.org/ticket/9580#comment:1
     83        info = {}
    7284        for provider in self.providers:
    7385            for name in provider.get_theme_names():
    7486                theme = provider.get_theme_info(name)
    7587                theme['provider'] = provider
    7688                theme['module'] = provider.__class__.__module__
    7789                theme['name'] = name
    78                 self.info[name.lower()] = theme
    79                
     90                info[name.lower()] = theme
     91        return info
     92
     93    if lazy is not None:
     94        info = lazy(info)
     95
    8096    # IThemeProvider methods
    8197    def get_theme_names(self):
    8298        yield 'default'

I'm using lazy instead of property because computation needs to be preformed just once . I'll commit this solution immediately after I confirm that everything will work with aforementioned versions . I've only tested modifications against Trac 0.13dev-r11046 so far . Anyway feedback is welcome .

comment:14 Changed 2 years ago by olemis

  • Status changed from new to assigned

comment:15 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by rjollos

Replying to olemis:

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

Isn't the patch actually fixing Trac >= 1.0 compatibility? The lazy class wasn't available until 1.0 anyway, so on Trac < 1.0, lazy will evaluate to None in __init__.py, and it looks like you'll be falling back to the old way of initializing the attribute. I think this part of the patch is fine, just the comments need to be updated to change reference to 0.12 to 1.0.

I'm using lazy instead of property because computation needs to be preformed just once.

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

comment:16 in reply to: ↑ 15 ; follow-up: Changed 2 years ago by olemis

Replying to rjollos:

Replying to olemis:

I'm proposing the following patch against trunk (see r12162 ) to make it work with Trac >= 0.11 . I'm also raising minor version number (i.e. 2.0.1 => 2.1.0 ) to delimit the exact point before and after Trac >= 0.12 compatibility .

Isn't the patch actually fixing Trac >= 1.0 compatibility?

It seems plugin should fail also with Trac=0.12 , so that's something to consider as well .

The lazy class wasn't available until 1.0 anyway, so on Trac < 1.0, lazy will evaluate to None in __init__.py, and it looks like you'll be falling back to the old way of initializing the attribute.

Yes I just checked few minutes ago and it seems lazy is not available in 0.12 . So I'll wait for a while before commiting, at least until somebody actually confirms whether this bug can still be reproduced with Trac=0.12 or not .

I think this part of the patch is fine, just the comments need to be updated to change reference to 0.12 to 1.0.

The solution should cover them all 0.11 , 0.12 and 1.0 . Besides I don't want to include copies of lazy nor any other function / class / ... defined in Trac .

I'm using lazy instead of property because computation needs to be preformed just once.

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

No , because that will fail for Trac<=0.11 .

comment:17 in reply to: ↑ 16 Changed 2 years ago by rjollos

Replying to olemis:

This is not an aspect of Python that I've used a lot, but can't you use lazy as a decorator, as suggest in comment:8?

No , because that will fail for Trac<=0.11 .

Okay, yes, I should have noticed that. But you must mean Trac < 1.0, because as you mentioned earlier, the lazy class was introduced in 1.0.

comment:18 Changed 2 years ago by rjollos

I've tested out the ThemeEnginePlugin with PyTppThemePlugin, and ThemeEnginePlugin 2.0.1 seems to work fine on Trac 0.12. I've also tested after the patch for ThemeEnginePlugin 2.1.0 has been applied and that works fine with Trac 0.12. t:#9418 has the milestone of 1.0 and none of the changes were merged to 0.12-stable, so I think this all makes sense. The issue does not seem to exist in Trac 0.12, and was introduced with Trac 1.0.

Finally, I've tested with Trac 1.0 just to confirm that I can reproduce the issue and wasn't overlooking something when testing with Trac 0.12. I don't see how I could be overlooking the presence of this issue ;)

My only suggestion is to change the two comments in the patch to make reference to Trac 1.0 rather than Trac 0.12.

  • trunk/themeengine/api.py

     
    1313from trac.core import *
    1414from trac.config import Option
    1515
     16try:
     17    from trac.util import lazy
     18except ImportError:
     19    lazy = None
     20
    1621class ThemeNotFound(TracError):
    1722    """The requested theme isn't found."""
    1823   
     
    6772    providers = ExtensionPoint(IThemeProvider)
    6873   
    6974    def __init__(self):
    70         # This can safely go in here because the data can only change on a restart anyway
    71         self.info = {}
     75        if lazy is None:
     76            # Trac < 1.0: this can safely go in here because the data can
     77            # only change on a restart anyway
     78            self.info = self.info()
     79
     80    def info(self):
     81        # Trac >= 1.0 : Hack needed to deal with infinite recusrion error
     82        #    Details : http://trac-hacks.org/ticket/9580#comment:1
     83        info = {}
    7284        for provider in self.providers:
    7385            for name in provider.get_theme_names():
    7486                theme = provider.get_theme_info(name)
    7587                theme['provider'] = provider
    7688                theme['module'] = provider.__class__.__module__
    7789                theme['name'] = name
    78                 self.info[name.lower()] = theme
    79                
     90                info[name.lower()] = theme
     91        return info
     92
     93    if lazy is not None:
     94        info = lazy(info)
     95
    8096    # IThemeProvider methods
    8197    def get_theme_names(self):
    8298        yield 'default'

comment:15 follow-up: Changed 2 years ago by olemis

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12173]) ThemeEnginePlugin: Fixes #9580 infinite recursion issue in Trac=1.0

Add Comment

Modify Ticket

Action
as closed The owner will remain olemis.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.