On Jan 30, 2012, at 5:22 PM, Marek Posolda wrote: > Hi, > > I think that it's not ideal how WebAppController processes static > resources like images. I have 2 things to point out: > > 1) Today I found (and fixed) a bug that images are first processed by > PortalRequestHandler, even if they should be processed by > StaticResourceRequestHandler. Please look at > https://issues.jboss.org/browse/GTNPORTAL-2338 for more details. This is > really not good because HTTP header "Cache-Control: no-cache" is > returned for every image, and so web browser needs to download images > for each HTTP request to portal. I fixed it by adding > StaticResourceRequestHandler before PortalRequestHandler into > navController configuration, so it should not be the issue anymore. in StaticResourceRequestHandler the request is dispatched with the servlet API and the "default" servlet should setup the proper headers. could you explain me why having the StaticResourceRequestHandler after the PortalRequestHandler prevents it ? > > 2) Second thing is not so bad but a bit more tricky. The piece of code > in WebAppController: > > {code} > if (!started) > { > > RequestLifeCycle.begin(ExoContainerContext.getCurrentContainer()); > started = true; > } > ..... > processed = handler.execute(new ControllerContext(this, > router, req, res, parameters)); > ..... > if (started) > { > RequestLifeCycle.end(); > } > {code} > > This means that we need to start RequestLifeCycle for processing of > every resource including static resource. And startup of some services > is quite expensive (like startup of OrganizationService requires startup > of Hibernate transaction). In other words, currently we are starting > Hibernate transaction for processing images and other static resources. > > It's obvious that some handlers (PortalRequestHandler, > LegacyRequestHandler and probably some others) really need to start > RequestLifecycle, but some others like StaticResourceRequestHandler > don't need it. So how about postponing the RequestLifecycle.begin from > WebAppController to concrete handlers, which really need it? And use > some ThreadLocal variable or flag "started" on ControllerContext, so > that we can track if RequestLifeCycle.begin has been already started > (can be useful to avoid double-start if some resource needs to be > processed by more request handlers) ? please make an abstract method on WebRequestHandler : protected abstract boolean getRequiresLifeCycle() that the WebAppController can use in the loop to replace the condition checked by if (!started) by the condition if (!started && handler.getRequiresLifeCycle()) { ... } (which requires to move this block in the if (handler != null) { } block

> > Any other suggestions for this? > > Marek > > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/gatein-dev

On 30.1.2012 23:33, Julien Viet wrote: > On Jan 30, 2012, at 7:45 PM, Julien Viet wrote: > >> On Jan 30, 2012, at 5:22 PM, Marek Posolda wrote: >> >>> Hi, >>> >>> I think that it's not ideal how WebAppController processes static >>> resources like images. I have 2 things to point out: >>> >>> 1) Today I found (and fixed) a bug that images are first processed by >>> PortalRequestHandler, even if they should be processed by >>> StaticResourceRequestHandler. Please look at >>> https://issues.jboss.org/browse/GTNPORTAL-2338 for more details. This is >>> really not good because HTTP header "Cache-Control: no-cache" is >>> returned for every image, and so web browser needs to download images >>> for each HTTP request to portal. I fixed it by adding >>> StaticResourceRequestHandler before PortalRequestHandler into >>> navController configuration, so it should not be the issue anymore. >> in StaticResourceRequestHandler the request is dispatched with the servlet API and the "default" servlet should setup the proper headers. >> >> could you explain me why having the StaticResourceRequestHandler after the PortalRequestHandler prevents it ? "default" servlet dispatched from StaticResourceRequestHandler does not add headers. Headers for images (like "Cache-control" header) are added by ResourceRequestFilter. Problem is that header is overriden by PortalRequestHandler.

Well, what's going during processing of HTTP request to image (like http://localhost:8080/portal/something.jpg ) before fix of GTNPORTAL-2338 is: - Header "Cache-Control: max-age=2592000,s-maxage=2592000" is added by ResourceRequestFilter - Processing is dispatched to WebAppController and there are two compliant handlers: PortalRequestHandler and StaticResourceRequestHandler - PortalRequestHandler is first and so image is processed first by it. And this does not makes sense as it's overriding Cache-Control header with value "no-cache" and it's calling some operations to UserPortalConfigService and DataStorage (really does not makes sense). - Processing by PortalRequestHandler returns false and so it continues to StaticResourceRequestHandler. This handler dispatches it to "default" servlet and image is returned to client, but with incorrect header "Cache-control: no-cache" The result is that every browser request to portal like "http://localhost:8080/portal/classic" needs to download all portal images as they can't be cached on the browser side. Really bandwidth killer IMO.

By adding StaticResourceRequestHandler before PortalRequestHandler is processing of images by PortalRequestHandler omited. For details please look at https://issues.jboss.org/browse/GTNPORTAL-2338 . >>> 2) Second thing is not so bad but a bit more tricky. The piece of code >>> in WebAppController: >>> >>> {code} >>> if (!started) >>> { >>> >>> RequestLifeCycle.begin(ExoContainerContext.getCurrentContainer()); >>> started = true; >>> } >>> ..... >>> processed = handler.execute(new ControllerContext(this, >>> router, req, res, parameters)); >>> ..... >>> if (started) >>> { >>> RequestLifeCycle.end(); >>> } >>> {code} >>> >>> This means that we need to start RequestLifeCycle for processing of >>> every resource including static resource. And startup of some services >>> is quite expensive (like startup of OrganizationService requires startup >>> of Hibernate transaction). In other words, currently we are starting >>> Hibernate transaction for processing images and other static resources. >>> >>> It's obvious that some handlers (PortalRequestHandler, >>> LegacyRequestHandler and probably some others) really need to start >>> RequestLifecycle, but some others like StaticResourceRequestHandler >>> don't need it. So how about postponing the RequestLifecycle.begin from >>> WebAppController to concrete handlers, which really need it? And use >>> some ThreadLocal variable or flag "started" on ControllerContext, so >>> that we can track if RequestLifeCycle.begin has been already started >>> (can be useful to avoid double-start if some resource needs to be >>> processed by more request handlers) ? >> please make an abstract method on WebRequestHandler : >> >> protected abstract boolean getRequiresLifeCycle() that the WebAppController can use in the loop to replace the condition checked by >> >> if (!started) >> >> by the condition >> >> if (!started&& handler.getRequiresLifeCycle()) { ... } >> >> (which requires to move this block in the >> >> if (handler != null) >> { >> } >> >> block > I just want that the whole request lifecycle stuff keeps confined in the WebAppController class if we can, as the subclass will likely always or never need a RequestLifecyle to be triggered. Thanks, initially I though that some handlers may need to start only something (for example start only ChromatticManager but not OrganizationService) but probably does not makes sense to complicate things because of it. So I've made it as you suggested. Jira is https://issues.jboss.org/browse/GTNPORTAL-2340 I made it that getRequiresLifeCycle returns true for: PortalRequestHandler, LegacyRequestHandler and returns false for: DefaultRequestHandler, StaticResourceRequestHandler, DownloadHandler, UploadHandler Adding new method will require changes for other handlers in other eXo components. At least for EPP-SP I am seeing two additional handlers mapped with names "robots" and "sitemap". Should I create JIRA for you in some eXo project to cover it?

>>> Any other suggestions for this? >>> >>> Marek >>> >>> _______________________________________________ >>> gatein-dev mailing list >>> gatein-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/gatein-dev >

On 31.1.2012 13:55, Julien Viet wrote: > On Jan 31, 2012, at 1:46 PM, Marek Posolda wrote: > >> On 30.1.2012 23:33, Julien Viet wrote: >>> On Jan 30, 2012, at 7:45 PM, Julien Viet wrote: >>> >>>> On Jan 30, 2012, at 5:22 PM, Marek Posolda wrote: >>>> >>>>> Hi, >>>>> >>>>> I think that it's not ideal how WebAppController processes static >>>>> resources like images. I have 2 things to point out: >>>>> >>>>> 1) Today I found (and fixed) a bug that images are first processed by >>>>> PortalRequestHandler, even if they should be processed by >>>>> StaticResourceRequestHandler. Please look at >>>>> https://issues.jboss.org/browse/GTNPORTAL-2338 for more details. This is >>>>> really not good because HTTP header "Cache-Control: no-cache" is >>>>> returned for every image, and so web browser needs to download images >>>>> for each HTTP request to portal. I fixed it by adding >>>>> StaticResourceRequestHandler before PortalRequestHandler into >>>>> navController configuration, so it should not be the issue anymore. >>>> in StaticResourceRequestHandler the request is dispatched with the servlet API and the "default" servlet should setup the proper headers. >>>> >>>> could you explain me why having the StaticResourceRequestHandler after the PortalRequestHandler prevents it ? >> "default" servlet dispatched from StaticResourceRequestHandler does not add headers. Headers for images (like "Cache-control" header) are added by ResourceRequestFilter. Problem is that header is overriden by PortalRequestHandler. > makes sense > > actually at some point the ResourceRequestFilter should be removed as it is intrusive for the war file. We have a prototype of resource serving for Javascript (spec is available here https://wiki-int.exoplatform.org/display/rhcollab/Resource+Controller) that makes all serving of resource to be done by the portal for deployed scripts. > >> Well, what's going during processing of HTTP request to image (like http://localhost:8080/portal/something.jpg ) before fix of GTNPORTAL-2338 is: >> - Header "Cache-Control: max-age=2592000,s-maxage=2592000" is added by ResourceRequestFilter >> - Processing is dispatched to WebAppController and there are two compliant handlers: PortalRequestHandler and StaticResourceRequestHandler >> - PortalRequestHandler is first and so image is processed first by it. And this does not makes sense as it's overriding Cache-Control header with value "no-cache" and it's calling some operations to UserPortalConfigService and DataStorage (really does not makes sense). >> - Processing by PortalRequestHandler returns false and so it continues to StaticResourceRequestHandler. This handler dispatches it to "default" servlet and image is returned to client, but with incorrect header "Cache-control: no-cache" >> >> The result is that every browser request to portal like "http://localhost:8080/portal/classic" needs to download all portal images as they can't be cached on the browser side. Really bandwidth killer IMO. > correct, can you fix it in gatein trunk with the same headers ?

I've fixed it in GateIn by adding StaticResourceRequestHandler before PortalRequestHandler. See https://issues.jboss.org/browse/GTNPORTAL-2338. > soon also we are going to investigate CSS sprite generation in order to pack several images into a single one to optimize the connections made to the server. > >> By adding StaticResourceRequestHandler before PortalRequestHandler is processing of images by PortalRequestHandler omited. For details please look at https://issues.jboss.org/browse/GTNPORTAL-2338 . >>>>> 2) Second thing is not so bad but a bit more tricky. The piece of code >>>>> in WebAppController: >>>>> >>>>> {code} >>>>> if (!started) >>>>> { >>>>> >>>>> RequestLifeCycle.begin(ExoContainerContext.getCurrentContainer()); >>>>> started = true; >>>>> } >>>>> ..... >>>>> processed = handler.execute(new ControllerContext(this, >>>>> router, req, res, parameters)); >>>>> ..... >>>>> if (started) >>>>> { >>>>> RequestLifeCycle.end(); >>>>> } >>>>> {code} >>>>> >>>>> This means that we need to start RequestLifeCycle for processing of >>>>> every resource including static resource. And startup of some services >>>>> is quite expensive (like startup of OrganizationService requires startup >>>>> of Hibernate transaction). In other words, currently we are starting >>>>> Hibernate transaction for processing images and other static resources. >>>>> >>>>> It's obvious that some handlers (PortalRequestHandler, >>>>> LegacyRequestHandler and probably some others) really need to start >>>>> RequestLifecycle, but some others like StaticResourceRequestHandler >>>>> don't need it. So how about postponing the RequestLifecycle.begin from >>>>> WebAppController to concrete handlers, which really need it? And use >>>>> some ThreadLocal variable or flag "started" on ControllerContext, so >>>>> that we can track if RequestLifeCycle.begin has been already started >>>>> (can be useful to avoid double-start if some resource needs to be >>>>> processed by more request handlers) ? >>>> please make an abstract method on WebRequestHandler : >>>> >>>> protected abstract boolean getRequiresLifeCycle() that the WebAppController can use in the loop to replace the condition checked by >>>> >>>> if (!started) >>>> >>>> by the condition >>>> >>>> if (!started&& handler.getRequiresLifeCycle()) { ... } >>>> >>>> (which requires to move this block in the >>>> >>>> if (handler != null) >>>> { >>>> } >>>> >>>> block >>> I just want that the whole request lifecycle stuff keeps confined in the WebAppController class if we can, as the subclass will likely always or never need a RequestLifecyle to be triggered. >> Thanks, initially I though that some handlers may need to start only something (for example start only ChromatticManager but not OrganizationService) but probably does not makes sense to complicate things because of it. So I've made it as you suggested. Jira is https://issues.jboss.org/browse/GTNPORTAL-2340 >> >> I made it that getRequiresLifeCycle returns true for: PortalRequestHandler, LegacyRequestHandler >> and returns false for: DefaultRequestHandler, StaticResourceRequestHandler, DownloadHandler, UploadHandler >> >> Adding new method will require changes for other handlers in other eXo components. At least for EPP-SP I am seeing two additional handlers mapped with names "robots" and "sitemap". Should I create JIRA for you in some eXo project to cover it? > yes you should but you should also create a JIRA in EXOGTN to port this change to our product branch. I've created https://jira.exoplatform.org/browse/SPUB-139 for EPPSP handlers. Seems that I don't have permission to create JIRA in EXOGTN project.

>>>>> Any other suggestions for this? >>>>> >>>>> Marek >>>>> >>>>> _______________________________________________ >>>>> gatein-dev mailing list >>>>> gatein-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/gatein-dev >