Jetty

Support HttpSession proxying to allow portlet bridges to provide "scoped" session view to dispatched servlets

Details

  • Type: Improvement Improvement
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 6.1.4
  • Fix Version/s: 6.1.5rc0
  • Component/s: Servlet
  • Labels:
    None
  • Environment:
    JDK 1.5, Jetty 6.1.4, Jetspeed-2.1.1-dev
  • Testcase included:
    yes
  • Patch Submitted:
    Yes
  • Number of attachments :
    2

Description

Apache Portals Bridges provides a neat solution (I wrote myself ) to "scope" HttpSession attributes for a specific Portlet window when it dispatches to a servlet.

See: http://svn.apache.org/viewvc/portals/bridges/trunk/common/src/java/org/apache/portals/bridges/util/ServletPortletSessionProxy.java?view=markup

This solution is needed for transparent "bridging" of existing web development frameworks like Struts, or Wicket (which I'm currently working on) to portlets.

So far, this proxy solution works like a charm and I've tested it successfully on Tomcat, Websphere, WebLogic and GlassFish.
Only with Jetty it fails

The reason is simple: AbstractSessionManager uses direct type casting of a provided HttpSession parameter to its inner Session class, so when a proxied version is passed in a ClassCastException is the result.

But the solution is also very simple, small and completely non-intrusive and I've attached a patch for it.
It adds a new interface JettySession with one method: JettySession getJettySession() and applies it to AbstractSessionManager.Session.
That interface allows Jetty to always get back to its own/underlying Session class.

With this patch applied, I can now run Jetspeed-2.1.1 (trunk) fully on Jetty 6.1.4 and we're planning to provide a Jetty based Jetspeed-2 demo installer with the imminent 2.1.1 release including neat hot deployment of war support.. But we will need to provide our own patched version of Jetty until this issue can be resolved.
(NB: I'm also a Jetspeed-2 committer)

So I would very much appreciate if you can review and consider this patch for inclusion in the next version of Jetty so we won't need to use a patched version of Jetty for running Jetspeed-2.

Regards,

Ate

  1. Text File
    patch.txt
    22/Jun/07 5:44 AM
    5 kB
    Ate Douma
  2. Text File
    patch2.txt
    22/Jun/07 8:21 AM
    3 kB
    Ate Douma

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Ate Douma added a comment -

The Jetspeed-2 - Jetty support I'm working on can be tracked here: http://issues.apache.org/jira/browse/JS2-736

Show
Ate Douma added a comment - The Jetspeed-2 - Jetty support I'm working on can be tracked here: http://issues.apache.org/jira/browse/JS2-736
Hide
Permalink
Greg Wilkins added a comment -

I am certainly open to change jetty to make this work.

But I think we can do this without the special interface.

instead of calling through an interface, could you try some code like this

Session s=(session instanceof Session)?(Session)session:getHttpSession(session.getId());

if that works... it will be a little slower... and we may need to do a fiddle for node/cluster id's

Show
Greg Wilkins added a comment - I am certainly open to change jetty to make this work. But I think we can do this without the special interface. instead of calling through an interface, could you try some code like this Session s=(session instanceof Session)?(Session)session:getHttpSession(session.getId()); if that works... it will be a little slower... and we may need to do a fiddle for node/cluster id's
Hide
Permalink
Ate Douma added a comment -

Ok, I will give that a try and let you know if it works.

But as you said: it's (a lot) slower, more complex and more risks for errors.
Why would you prefer that above my proposed interface (which is only seen/used internally)?

Show
Ate Douma added a comment - Ok, I will give that a try and let you know if it works. But as you said: it's (a lot) slower, more complex and more risks for errors. Why would you prefer that above my proposed interface (which is only seen/used internally)?
Hide
Permalink
Ate Douma added a comment -

Yes, your solution works too, patch2.txt attached.
And I can see the benefit of doing it this way as it allows for more use-cases, like wrapping HttpSession instead of using a proxy.

I'm fine with either solution

Thanks for the quick response.

Show
Ate Douma added a comment - Yes, your solution works too, patch2.txt attached. And I can see the benefit of doing it this way as it allows for more use-cases, like wrapping HttpSession instead of using a proxy. I'm fine with either solution Thanks for the quick response.
Hide
Permalink
Greg Wilkins added a comment -

Actually, I have thought of a third way that is fast.
I will use the getAttribute(String) method and do an == check on a name of "org.mortbay.jetty.servlet.Session"
and if it matches, I will pass back the session.

So the line is now:

Session s = (Session)((session instanceof Session)?session:session.getAttribute(SESSION_ATTR));

because it is an == check, it is fast
because the string is private static, no other session user can get it.

So it is a perfect way to tunnel through any wrappers/proxies.

I have implemented this and will check in after a bit more testing.

cheers

Show
Greg Wilkins added a comment - Actually, I have thought of a third way that is fast. I will use the getAttribute(String) method and do an == check on a name of "org.mortbay.jetty.servlet.Session" and if it matches, I will pass back the session. So the line is now: Session s = (Session)((session instanceof Session)?session:session.getAttribute(SESSION_ATTR)); because it is an == check, it is fast because the string is private static, no other session user can get it. So it is a perfect way to tunnel through any wrappers/proxies. I have implemented this and will check in after a bit more testing. cheers
Hide
Permalink
Ate Douma added a comment -

Although through session.getAttributeNames() someone could get to this "private" attribute and thus theoretically even remove it, they're self to blame then I guess.
So for me its a fine solution too.

Show
Ate Douma added a comment - Although through session.getAttributeNames() someone could get to this "private" attribute and thus theoretically even remove it, they're self to blame then I guess. So for me its a fine solution too.
Hide
Permalink
Greg Wilkins added a comment -

Ate,
this is not a real attribute and is not listed in the names, can't be replaced and does not use memory/cycles in the attribute map.

It is a common technique used by most container implementations to expose private APIs
via attributes.

This is checked in now and it would be good to know if this is working for you.
I am pushing a maven snapshot at the moment.

Show
Greg Wilkins added a comment - Ate, this is not a real attribute and is not listed in the names, can't be replaced and does not use memory/cycles in the attribute map. It is a common technique used by most container implementations to expose private APIs via attributes. This is checked in now and it would be good to know if this is working for you. I am pushing a maven snapshot at the moment.
Hide
Permalink
Ate Douma added a comment -

Ok, that's cool.

I'll try to test it out today and let you know.

Show
Ate Douma added a comment - Ok, that's cool. I'll try to test it out today and let you know.
Hide
Permalink
Ate Douma added a comment -

Sorry for the delay, I'm planning to test it out tomorrow.

Show
Ate Douma added a comment - Sorry for the delay, I'm planning to test it out tomorrow.
Hide
Permalink
Ate Douma added a comment -

Sorry Greg for getting back on this so late (we've been very busy preparing a new Jetspeed 2.1.2 release), but I'm sorry to say your solution is not working.
And in fact (and I should have recognized this earlier), using a hardcoded attribute name is exactly what is not possible for this purpose.

This issue specifically concerns scoping session attributes to allow isolated views by multiple portlet instances.
The way we provide this scoped view is intercepting the get/set attribute methods with a Proxy class and prefixing the attribute names with a scope name (e.g. portletWindowId).

The problem with the solution you've committed is that a session.getAttribute("org.mortbay.jetty.servlet.Session") call made on our Proxy instance is at the end delegated to the real Jetty Session as getAttribute("<portletWindowId>_org.mortbay.jetty.servlet.Session"), with as result an NPE in the subsequent usage of the (not) retrieved real session instance:

java.lang.NullPointerException
at org.mortbay.jetty.servlet.AbstractSessionManager.complete(AbstractSessionManager.java:146)
at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:194)
at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:712)
at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405)
at org.mortbay.jetty.servlet.Dispatcher.include(Dispatcher.java:192)
at org.apache.portals.bridges.struts.StrutsPortlet.processRequest(StrutsPortlet.java:427)
at org.apache.portals.bridges.struts.StrutsPortlet.doView(StrutsPortlet.java:301)
at javax.portlet.GenericPortlet.doDispatch(GenericPortlet.java:247)
at javax.portlet.GenericPortlet.render(GenericPortlet.java:175)
at org.apache.jetspeed.factory.JetspeedPortletInstance.render(JetspeedPortletInstance.java:103)

So could you please review (again) my original proposal of adding an JettySession interface to AbstractSessionManager.Session?
It is a solution which is 100% save (and tested already) and I think not very expensive either.

Show
Ate Douma added a comment - Sorry Greg for getting back on this so late (we've been very busy preparing a new Jetspeed 2.1.2 release), but I'm sorry to say your solution is not working. And in fact (and I should have recognized this earlier), using a hardcoded attribute name is exactly what is not possible for this purpose. This issue specifically concerns scoping session attributes to allow isolated views by multiple portlet instances. The way we provide this scoped view is intercepting the get/set attribute methods with a Proxy class and prefixing the attribute names with a scope name (e.g. portletWindowId). The problem with the solution you've committed is that a session.getAttribute("org.mortbay.jetty.servlet.Session") call made on our Proxy instance is at the end delegated to the real Jetty Session as getAttribute("<portletWindowId>_org.mortbay.jetty.servlet.Session"), with as result an NPE in the subsequent usage of the (not) retrieved real session instance: java.lang.NullPointerException at org.mortbay.jetty.servlet.AbstractSessionManager.complete(AbstractSessionManager.java:146) at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:194) at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:712) at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405) at org.mortbay.jetty.servlet.Dispatcher.include(Dispatcher.java:192) at org.apache.portals.bridges.struts.StrutsPortlet.processRequest(StrutsPortlet.java:427) at org.apache.portals.bridges.struts.StrutsPortlet.doView(StrutsPortlet.java:301) at javax.portlet.GenericPortlet.doDispatch(GenericPortlet.java:247) at javax.portlet.GenericPortlet.render(GenericPortlet.java:175) at org.apache.jetspeed.factory.JetspeedPortletInstance.render(JetspeedPortletInstance.java:103) So could you please review (again) my original proposal of adding an JettySession interface to AbstractSessionManager.Session? It is a solution which is 100% save (and tested already) and I think not very expensive either.
Hide
Permalink
Greg Wilkins added a comment -

OK... I hate making others implement jetty specific interfaces, but if you are happy to do so...

I have checked in the change, but the interface is nested :

AbstractSessionManager.SessionIf

this will be in jetty 6.1.5rc0 this week.

thanks for persisting.

Show
Greg Wilkins added a comment - OK... I hate making others implement jetty specific interfaces, but if you are happy to do so... I have checked in the change, but the interface is nested : AbstractSessionManager.SessionIf this will be in jetty 6.1.5rc0 this week. thanks for persisting.
Hide
Permalink
Ate Douma added a comment -

Great, I will try to test it tomorrow.

FYI, this interface is not needed to be implemented or used (directly) by anyone outside the AbstractSessionManager itself.
We use a dynamic proxy which just proxies any interface the session object has.
That way, calling the interface method to get back to the real instance simply passes through the proxy, but the "client" code never really needs to know, nor wants to care

Show
Ate Douma added a comment - Great, I will try to test it tomorrow. FYI, this interface is not needed to be implemented or used (directly) by anyone outside the AbstractSessionManager itself. We use a dynamic proxy which just proxies any interface the session object has. That way, calling the interface method to get back to the real instance simply passes through the proxy, but the "client" code never really needs to know, nor wants to care
Hide
Permalink
Ate Douma added a comment -

Confirmed this now works fine.
Thanks a lot Greg for accepting the patch.

This allows us now to declare Jetspeed-2 to work 100% on Jetty 6.1.5+.
After our Jetspeed 2.1.2 release (planned this weekend), I will add Jetty based installation support out-of-the-box!

Regards,

Ate

Show
Ate Douma added a comment - Confirmed this now works fine. Thanks a lot Greg for accepting the patch. This allows us now to declare Jetspeed-2 to work 100% on Jetty 6.1.5+. After our Jetspeed 2.1.2 release (planned this weekend), I will add Jetty based installation support out-of-the-box! Regards, Ate

People

  • Assignee:
    Greg Wilkins
    Reporter:
    Ate Douma
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: