Jetty
  1. Jetty
  2. JETTY-377

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. patch.txt
        5 kB
        Ate Douma
      2. patch2.txt
        3 kB
        Ate Douma

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: