Create Ticket
Warning Can't synchronize with repository "(default)" (Couldn't open Subversion repository /x1/svn/asf/bloodhound: SubversionException: ("Expected FS format between '1' and '4'; found format '6'", 160043)). Look in the Trac log for more information.

Reporter:
peter  
Opened:
Type:
enhancement  
Status:
closed : fixed  
Priority:
minor  
Milestone:
Component:
Version:
 
Description (last modified by peter) (diff)

For optimal performance req.chrome should be evaluated only if necessary.
This currently violated on multiple places in request handlers, filters and mainly by TracThemeEngine? plugin.

BH's request handlers and filters could be fixed within BH (whitelabeling solution was already dicussed on dev list). However there has been no investigation on TracThemeEngine? yet. Until this is fixed there is no benefits in fixing BH code at all.

Cc:
 

Attachments

TracErrorAfterRevision1437967.png (44.7 KB) - added by rjollos 15 months ago.
RequestUriTooLarge.png (7.4 KB) - added by rjollos 15 months ago.

Download all attachments as: .zip


Change History

peter

  • Description modified (diff)
  • Priority changed from major to minor
16 months ago

comment:2  

In reply to: ↑ description

Follow-up:

olemis

Replying to peter:

For optimal performance req.chrome should be evaluated only if necessary.
This currently violated on multiple places in request handlers, filters and mainly by trachacks:TracThemeEngine plugin.

Could you please mention where is it exactly that trachacks:ThemeEnginePlugin violates lazy instantiation of req.chrome ?

[...]

However there has been no investigation on trachacks:TracThemeEngine yet. Until this is fixed there is no benefits in fixing BH code at all.

I'm the maintainer of that plugin . I've not noticed something like that , so I ask : What's wrong exactly ?

16 months ago

comment:3  

In reply to: ↑ 2

Follow-up:

peter

Replying to olemis:

Replying to peter:

For optimal performance req.chrome should be evaluated only if necessary.
This currently violated on multiple places in request handlers, filters and mainly by trachacks:TracThemeEngine plugin.

Could you please mention where is it exactly that trachacks:ThemeEnginePlugin violates lazy instantiation of req.chrome ?

[...]

However there has been no investigation on trachacks:TracThemeEngine yet. Until this is fixed there is no benefits in fixing BH code at all.

I'm the maintainer of that plugin . I've not noticed something like that , so I ask : What's wrong exactly ?

When the plugin is installed and active it's post_process_request() will be invoked for every request. AFAICT there is nothing in that code that would differentiate requests (API vs UI requests) so add_stylesheet() will be called for all of them which then calls add_link() that evaluates req.chrome.

There might be other cases as well (maybe in Trac itself also) but this is the one I stumbled upon when I was investigating why does my solution for lazy whitelabeling have no effect at all.

16 months ago

comment:4  

In reply to: ↑ 3

Follow-up:

olemis

Replying to peter:

Replying to olemis:

Replying to peter:

For optimal performance req.chrome should be evaluated only if necessary.
This currently violated on multiple places in request handlers, filters and mainly by trachacks:TracThemeEngine plugin.

Could you please mention where is it exactly that trachacks:ThemeEnginePlugin violates lazy instantiation of req.chrome ?

[...]

When the plugin is installed and active it's post_process_request() will be invoked for every request. AFAICT there is nothing in that code that would differentiate requests (API vs UI requests) so add_stylesheet() will be called for all of them which then calls add_link() that evaluates req.chrome.

I'll check that
;)

https://trac-hacks.org/ticket/10748

16 months ago

comment:5  

In reply to: ↑ 4

olemis

Replying to olemis:

Replying to peter:

[...]

When the plugin is installed and active it's post_process_request() will be invoked for every request. AFAICT there is nothing in that code that would differentiate requests (API vs UI requests) so add_stylesheet() will be called for all of them which then calls add_link() that evaluates req.chrome.

I'll check that
;)

https://trac-hacks.org/ticket/10748

In order for you to move forward do you need anything beyond changes committed in th:changeset:12508 ?

peter

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

Fix in r1437967

15 months ago

comment:7  

In reply to: ↑ 6

Follow-up:

olemis

Replying to peter:

Fix in r1437967

Notice that another check for sys.exc_info is needed (see this fix for ThemeEnginePlugin for further details) . Otherwise error pages seem not to get the proper styling info .

I was tempted to reopen this ticket , but I didn't . First we should check whether error pages will be broken after r1437967 .

rjollos

15 months ago

comment:8  

In reply to: ↑ 7

Follow-up:

rjollos

Replying to olemis:

Notice that another check for sys.exc_info is needed (see this fix for ThemeEnginePlugin for further details) . Otherwise error pages seem not to get the proper styling info .

I was tempted to reopen this ticket , but I didn't . First we should check whether error pages will be broken after r1437967 .

I had noticed the other day that TracError pages weren't rendering correctly. I tested again today in an environment created last week, and saw that the TracError page was rendering correctly, but the theme had gone away. I then created a new environment in order to get the latest version of ThemeEnginePlugin with Olemis' fixes, and now I see something that looks nice:


I invoked the TracError by replacing data with None in trac.wiki.web_ui._render_view. I wanted to force an exception when the template was rendered. Are there other ways good ways to test this?

Clicking the Create button leads to an interesting error:


15 months ago

comment:9  

In reply to: ↑ 8

Follow-up:

olemis

Replying to rjollos:

Replying to olemis:

Notice that another check for sys.exc_info is needed (see this fix for ThemeEnginePlugin for further details) . Otherwise error pages seem not to get the proper styling info .

I was tempted to reopen this ticket , but I didn't . First we should check whether error pages will be broken after r1437967 .

I had noticed the other day that TracError pages weren't rendering correctly. I tested again today in an environment created last week, and saw that the TracError page was rendering correctly, but the theme had gone away. I then created a new environment in order to get the latest version of ThemeEnginePlugin with Olemis' fixes, and now I see something that looks nice:

[...]

yes , that's correct . I noticed the issue recently .

I invoked the TracError by replacing data with None in trac.wiki.web_ui._render_view. I wanted to force an exception when the template was rendered.

That's not the kind of errors we are tracking in this ticket . Indeed , its goal is not to instantiate req.chrome IF there is no template processing involved .

OT

AFAICT if there's an error rendering a Genshi template then you'll get a plain-text error message . I guess that happens because the failure took place at a lower layer (i.e. Genshi rendering engine)

Are there other ways good ways to test this?

I usually just go to /ticket/whatever_that_fails ... I do not know if that's good enough though .

[...]

15 months ago

comment:10  

In reply to: ↑ 9

Follow-up:

rjollos

Replying to olemis:

I invoked the TracError by replacing data with None in trac.wiki.web_ui._render_view. I wanted to force an exception when the template was rendered.

That's not the kind of errors we are tracking in this ticket . Indeed , its goal is not to instantiate req.chrome IF there is no template processing involved .

Your comment was Otherwise error pages seem not to get the proper styling info ... First we should check whether error pages will be broken after r1437967., therefore I thought that as a side issue we needed to look at whether TracError pages were rendering correctly. This seemed relevant since I had noticed recently that they were not rendering correctly.

AFAICT if there's an error rendering a Genshi template then you'll get a plain-text error message.

My test case results in a genshi template rendering error, and in that case my screen capture shows that the TracError page is rendered. However, in other cases I only see a plain-text error message. I don't understand what conditions lead to the different views.

15 months ago

comment:11  

In reply to: ↑ 10

olemis

Replying to rjollos:

Replying to olemis:

I invoked the TracError by replacing data with None in trac.wiki.web_ui._render_view. I wanted to force an exception when the template was rendered.

That's not the kind of errors we are tracking in this ticket . Indeed , its goal is not to instantiate req.chrome IF there is no template processing involved .

Your comment was Otherwise error pages seem not to get the proper styling info ... First we should check whether error pages will be broken after r1437967., therefore I thought that as a side issue we needed to look at whether TracError pages were rendering correctly.

Yes , you are right . There was a lot of implicit knowledge in my previous statement .

Let's focus on trac.web.chrome.Chrome.render_template method . In there _post_process_request is called in three places .

  1. after request handler contributes any data to expand a given template (invoked with args template, data, content_type )
  2. when request handler sends data on its own (invoked with None, None, None )
  3. when an error is detected (invoked with None, None, None inside except block)

req.chrome needs to be instantiated only in case 1 and 3 and that's what this ticket is for . The way it is now , it's only taking into account 1 . Basic theme style will be there , but further items added while post processing the request will not . That was what I meant .
;)

This seemed relevant since I had noticed recently that they were not rendering correctly.

maybe yes , maybe not ...

AFAICT if there's an error rendering a Genshi template then you'll get a plain-text error message.

My test case results in a genshi template rendering error, and in that case my screen capture shows that the TracError page is rendered.

In that case req.chrome should be instantiated along the way .

However, in other cases I only see a plain-text error message. I don't understand what conditions lead to the different views.

Yes , I guess that depends on the exact point where the error happens e.g. while expanding variables , you'll get the error page (... but you'll get a plain text error if Genshi template cannot be found ? ) . It's a bit non-deterministic ; or at least it is beyond my current understanding .

andrej

I can see the similar behavior when rendering an error page on local env (rev 1439061). The error occurs when an exception is trigged during request dispatching by handler. For example, use built-in search and set page parameter that is out of range e.g. http://localhost:8000/main/search?q=test&page=100

You've got a plain text error page indicating that there is an error in

... bloodhound_theme.html", line 278, in <Expression u'chrome.labels.footer_left_prefix'>
... 
has no member named "labels"

As far as I understand the code, the problem is in bloodhound_theme/bhtheme/theme.py, line 217

    def post_process_request(self, req, template, data, content_type):
        """Post process request filter.
        Removes all trac provided css if required"""
        
        if template is None and data is None:
            return template, data, content_type
...
        req.chrome['labels'] = self._get_whitelabelling()

In case of an exception during request processing, the post_process_request method is called with all None parameters, so method returns before req.chrome['labels'] is filled.

I think we need another way to figure out when we can skip theme post processing. So far my suggestion is:

Index: bloodhound_theme/bhtheme/theme.py
===================================================================
--- bloodhound_theme/bhtheme/theme.py	(revision 1439076)
+++ bloodhound_theme/bhtheme/theme.py	(working copy)
@@ -214,9 +214,6 @@
         """Post process request filter.
         Removes all trac provided css if required"""
         
-        if template is None and data is None:
-            return template, data, content_type
-        
         def is_active_theme():
             is_active = False
             active_theme = ThemeEngineSystem(self.env).theme
Last edited 15 months ago by andrej (previous) (diff)
15 months ago

comment:13  

In reply to: ↑ 12

olemis

Replying to andrej:

I can see the similar behavior when rendering an error page on local env (rev 1439061). The error occurs when an exception is trigged during request dispatching by handler.

[...]

As far as I understand the code, the problem is in bloodhound_theme/bhtheme/theme.py, line 217

[...]

So yes , this confirms my previous suspicion .

In case of an exception during request processing, the post_process_request method is called with all None parameters, so method returns before req.chrome['labels'] is filled.

That's right . That's what I was talking about .

I think we need another way to figure out when we can skip theme post processing. So far my suggestion is:

Did you read my previous comment ? In there I mention how I fixed exactly the same issue , but for trachacks:ThemeEnginePlugin .

peter

The error page regression should now be fixed by r1439270

olemis

milestone = milestone:"Release 4" (not available as milestone seems to be closed)

rjollos

  • Milestone set to Release 4
Note: See TracTickets for help on using tickets.

Activity

  

Warning   No events reported for enhancement: Lazy evaluation of req.chrome (closed: fixed) in the last 30 days since Apr 20, 2014. This may happen if system is not configured correctly. Please contact your administrator if you think this is the case.