Pluto
  1. Pluto
  2. PLUTO-557

Fixing the RequestDispatcher implementation to be more web container generic and pluggable through a Service

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.0
    • Component/s: portlet container
    • Labels:
      None
    • Environment:
      Tomcat 5/6, Websphere 6.1

      Description

      The requirements of the new portlet 2.0 specification with respect to portlet request dispatching is one area where complexity has grown massively. Especially the new support for forwarding, even from a RenderRequest, which requires the portlet container to "fake" the dispatch state as a forward while internally it must use an include, is very difficult to implement generically.

      The current implementation as I wrote some months ago uses a solution by interpreting the expected dispatch state upfront, before doing the actual dispatch, and then use that state internally to present to the invoked Servlet (RequestDispatcherPathInfo).
      This solution however now turned out to be "not good enough"...

      While the Portlet 2.0 TCK is a great help of validating our container implementation, in certain areas it only goes about "1 inch deep".
      So, while the current implementation passes the TCK, its not difficult to come up with practical use-cases which break the implementation flat out
      One specific conveniently "ignored" servlet api usage in the TCK is using a ServletContext.getRequestDispatcher(path) from within a dispatched Servlet. While there are a few (minor) descriptions in JSR-286 how the dispatcher state should be managed and what is allowed and not, the TCK only tests using a ServletRequest.getRequestDispatcher(path)...
      The real problem of using the ServletContext.getRequestDispatcher is that the portlet container handling will not, and can not, be aware of that as the servlet api provides no means to intercept or wrap such usage!

      Another very important limitation of the current implementation is that it turned out to be very Tomcat specific!
      For instance, the "detection" if a dispatched request is further dispatched (nested) currently depends on how Tomcat deals with that.
      Tomcat always "injects" another Tomcat specific RequestHandler above the current request wrapper to handle nested dispatch specific state (like merged query string parameters on the parameter map and include/forward specific request path attributes).
      The current implementation checks if the getRequest() of the current request is changed, and if so rebuilds its internal parameterMap cache.

      However, while testing Pluto (or actually Jetspeed using Pluto) on Websphere 6.1, this turned out to be not working at all.
      Websphere (and like Jetty I thereafter discovered) dynamically update its initial RequestHandler instead.

      To cut a long story short: the current implementation is both broken and too much (Tomcat) web container specific.

      However, I've been working on an alternative solution during the last week which is much more generic and capable of handling much more use-cases (but not all: that will never be possible because of spec limitations).

      This new solution no longer tries to interpret expected dispatch state upfront, but instead derives this during/after a dispatch, including nested dispatch and ServletContext.getRequestDispatcher invocations.
      This solution no longer needs the servlet url mapping from web.xml as used by the current implementation (only), which means that part of the OM loading can be dropped again and removed from the pluto container api as well!

      I have tested this against both Tomcat 5/6 and Websphere 6.1 successfully, which includes the TCK tests and several other real usage test-cases.

      One downside of the new implementation is that it requires somewhat more runtime overhead, as it for instance requires comparing all (10) dispatch path attributes on the current request with a previously cached copy on each request attribute read access (for these attributes).
      However, I'm already working on a "adaptive" algorithm to automatically detect at runtime if a much more optimal nested dispatch detection as we used for Tomcat may be used too. If true, the implementation then can "switch" to using that detection mechanism instead.

      All the above however makes clear that we're dealing with a very tricky part of the portlet spec which might or might not work on other web containers than has been tested against Tomcat/Websphere so far.
      For that reason, I'm also reworking the current implementation towards a new container RequestDispatcherService to make it pluggable and extendable.
      The RequestDispatcherService rewrite already is mostly done, the "adaptive" nested dispatch detection is still in the works but should be completed pretty soon. When both are finished, I'll replace the current broken implementation with this new solution ASAP.

        Activity

        Hide
        Ate Douma added a comment -

        I'm finally ready to commit my changes for the above.

        The new solution provides:

        • A new RequestDispatcherService to allow for a pluggable and extendable Servlet RequestDispatcher implementation
          The HttpPortletServletRequestWrapper class functionality is split into fine-grained protected method and uses protected or package scope member fields to actually make it easy to extend
        • all HttpPortletServletRequestWrapper functionality is now heavily documented both inline and as javadoc
        • DispatchDetection (a static enum) for only getParameterMap is configurable:
        • CHECK_STATE (default): always compare current and previous parameterMap to detect changes
        • CHECK_REQUEST_WRAPPER_STACK: always use the request wrapper stack (size) to see if the dispatch level has changed
        • EVALUATE: upon the first getParameterMap call check if the request wrapper stack (size) is changed and switch to CHECK_REQUEST_WRAPPER_STACK, otherwise to CHECK_STATE
        • the default CHECK_STATE can be overridden as constructor parameter on the (default) RequestDispatcherServiceImpl
        • for detecting a dispatch on access to path method or attribute values, this solution can not be used
        • PortletApplicationDefininition.getServletMappingURLPattern and RequestDispatcherPathInfo/-Provider no longer are needed and removed again (see also: PLUTO-535)

        One more note about the DispatchDetection: while I put a lot of effort in making this configurable so we could retain our old (Tomcat specific) solution, I now think it most probably won't be much more efficient in practice.
        Only when you would have many servlet dispatches with many overriding query string parameters might this be more effective.
        But, in the more/most common use-cases, I see little of that, or with only a few parameters.
        And, if no parameters are used at all (also very common, especially during rendering) the CHECK_STATE solution actually will be much more efficient because comparing empty maps only takes a single statement.
        Anyway, the default now is CHECK_STATE already.
        If there is consensus however that the CHECK_REQUEST_WRAPPER_STACK doesn't add much value (anymore) after all, we could squeeze a little more efficiency out of this by removing all the conditional DispatchDetection handling.

        Show
        Ate Douma added a comment - I'm finally ready to commit my changes for the above. The new solution provides: A new RequestDispatcherService to allow for a pluggable and extendable Servlet RequestDispatcher implementation The HttpPortletServletRequestWrapper class functionality is split into fine-grained protected method and uses protected or package scope member fields to actually make it easy to extend all HttpPortletServletRequestWrapper functionality is now heavily documented both inline and as javadoc DispatchDetection (a static enum) for only getParameterMap is configurable: CHECK_STATE (default): always compare current and previous parameterMap to detect changes CHECK_REQUEST_WRAPPER_STACK: always use the request wrapper stack (size) to see if the dispatch level has changed EVALUATE: upon the first getParameterMap call check if the request wrapper stack (size) is changed and switch to CHECK_REQUEST_WRAPPER_STACK, otherwise to CHECK_STATE the default CHECK_STATE can be overridden as constructor parameter on the (default) RequestDispatcherServiceImpl for detecting a dispatch on access to path method or attribute values, this solution can not be used PortletApplicationDefininition.getServletMappingURLPattern and RequestDispatcherPathInfo/-Provider no longer are needed and removed again (see also: PLUTO-535 ) One more note about the DispatchDetection: while I put a lot of effort in making this configurable so we could retain our old (Tomcat specific) solution, I now think it most probably won't be much more efficient in practice. Only when you would have many servlet dispatches with many overriding query string parameters might this be more effective. But, in the more/most common use-cases, I see little of that, or with only a few parameters. And, if no parameters are used at all (also very common, especially during rendering) the CHECK_STATE solution actually will be much more efficient because comparing empty maps only takes a single statement. Anyway, the default now is CHECK_STATE already. If there is consensus however that the CHECK_REQUEST_WRAPPER_STACK doesn't add much value (anymore) after all, we could squeeze a little more efficiency out of this by removing all the conditional DispatchDetection handling.
        Hide
        Ate Douma added a comment -

        Committed

        Show
        Ate Douma added a comment - Committed

          People

          • Assignee:
            Ate Douma
            Reporter:
            Ate Douma
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development