[MAGNOLIA-6012] Ensure a page defined in 404-section of web.xml gets the channel Created: 03/Dec/14  Updated: 18/Aug/15  Resolved: 24/Feb/15

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 5.2.9, 5.3.6
Fix Version/s: 5.2.11, 5.3.8

Type: Bug Priority: Neutral
Reporter: Christoph Meier Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Cloners
is cloned by MAGNOLIA-6120 CLONE - Ensure a page defined in 404-... Closed
causality
caused by MAGNOLIA-5960 OncePerRequestAbstractMgnlFilter must... Closed
caused by MAGNOLIA-6073 Different instances of components aft... Closed
relation
is related to MAGNOLIA-5835 Filters that extend from OncePerReque... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

If you have a (magnolia) page defined in the web.xml as the 404 error, it does not get the channel.

This is a follow-up ticket of MAGNOLIA-5835; M-5835 was fixed but now has been reverted by MAGNOLIA-5960.
We should find a solution which will not trigger OncePerRequestAbstractMgnlFilter(s) twice.



 Comments   
Comment by Jan Haderka [ 10/Feb/15 ]

Would need to dig bit deeper to confirm my suspicion, but right now, and after quick test (which by no mean guarantees what i saw), I think that reason for different behaviour is that jetty will always reuse same thread for sendError processing, but i seemed to have managed, at least once, get Tomcat to use different worker thread for processing error. So while request object is reused (hence Magnolia knows filters were already processed), same is not guaranteed for the worker thread used to process the request and thus also not for the associated ThreadLocal. I could also not find same guarantee in the spec. Only thing it says is that there is always only one thread processing the request or in some incarnations also that when a request arrives, it is handled by the thread receiving the request. Either way there is no guarantee that after calling sendError and having server process this against web.xml specified error page it will reuse same thread. It might (Jetty) or not (Tomcat). So, in essence, (and imho) it is not safe to rely on thread local to always match that what is set in request attribute and where possible we should use request attribute rather than thread local.

As I see it right now (and above should still be confirmed by tests and more thorough read of the spec then my cursory glance) since we can't rely on ThreadLocal to match, we should either

  • clear all there request attributes related to once-per-request filters when calling MgnlContext.release()
    • feels dangerous, side effects need to be checked specially when using JSP components for templating
  • store whole AggregationState in request attribute rather than thread local
    • feels equally or more dangerous
  • make channel resolution related filters not to extend {{OncePerRequest...}
    • not sure why they did in the first place, there might be bigger consequences then just extra processing
    • solves the issue just for channel but not for anything else
  • store (and retrieve) AggregationState in both thread local and request attribute (if possible) for time being
    • feels just like workaround w/o really attacking the core of the problem, otoh might be find for bug fix release to minimise impact.
    • still does nothing for any other info stored in MgnlContext (and thus in ThreadLocal)

... in summary, I don't have a silver bullet for this one either

Comment by Christoph Meier [ 12/Feb/15 ]

When reviewing, check branch "MAGNOLIA-6012_5.3.x" on main and branch "MAGNOLIA-6012_integrationTest" on ce-bundle.

Comment by Mikaël Geljić [ 20/Feb/15 ]

W/ Greg we decided to go for the "no longer a OncePerRequestAbstractMgnlFilter" approach.

  • Initially I thought there could be problems in terms of binary compatibility — but turns out there are none.
  • We'll still limit potential side-effects by overriding the #bypasses method on the MultiChannelFilter and only run channel resolution if it's not set already.
Comment by Christoph Meier [ 24/Feb/15 ]

MultiChannelFilter now extends AbstractMgnlFilter to ensure channel is set on Tomcat container too.
#bypass is overridden to ensure the Filter is only executed if required.

All commits on branch "MAGNOLIA-6012_5.3.x"

Generated at Mon Feb 12 04:10:29 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.