Pluto
  1. Pluto
  2. PLUTO-569

Threading issue in DefaulltPortletInvoker

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.2, 2.1.0
    • Component/s: portal driver
    • Labels:
      None

      Description

      A portlet application with multiple portlets registered to listen for the same event often falls over with NPE exceptions in PortletServlet.dispatch() (line 308), caused by the PortletRequest not being set in the ServletRequest.

      On investigation, the issue appears to be that the DefaultPortletInvoker is sharing the containerRequest object across event-firing threads, and the invoking block is not synchronised. That is, event thread 1 adds the PortletRequest to the ServletRequest, does it's work, and then removes the PortletRequest. Meanwhile, event thread 2 is in the middle of doing it's work, but thread 1 has just removed the PortletRequest attribute.

      Zip file with portlets and test case demonstrating the issue (it occurs on about 40-50% of requests on an Intel Xeon 2.4Ghz, 2Gb RAM on Win XP) will be attached, along with patch to simply synchronize the relevant block on the ServletRequest object in DefaultPortletInvokerService.invoke().

      1. patch.txt
        6 kB
        Mark Piper
      2. PlutoIssueTestCase.zip
        5 kB
        Mark Piper

        Activity

        Hide
        Ate Douma added a comment -

        Replaced the multi-threaded event delivery with a plain in-process solution.

        Show
        Ate Douma added a comment - Replaced the multi-threaded event delivery with a plain in-process solution.
        Hide
        Ate Douma added a comment -

        Hi Mark,

        You assessment is spot on, but effectively you're patch trims down the multi-threaded event delivery into a single synchronised event handling, that is: for the most/critical part (the actual invocation of the portlet).
        However, synchronising only this part still won't make it a 100% bullet proof solution as even in the pre/post processing of the event handling it is possible to encounter threads overwriting part of others state, especially the request attributes.

        The real cause of this is that the Pluto Portal Driver doesn't properly manage request states per portlet window itself, but simplistically stores all request state (attributes) within the single underlying client HttpServletRequest.

        Please note: this is not an issue of the Pluto container implementation itself which is completely (request) stateless, which was one of the most critical goals of the Pluto 2.0 container redesign.
        FYI: in Jetspeed Portal 2.x we actively leverage this capability of the Pluto container for real thread-save parallel request processing and in Jetspeed we maintain the request state for each portlet window completely isolated from the underlying HttpServletRequest.

        Providing equally thread-save parallel request processing to the Pluto Portal Driver would require very extensive enhancements and redesign.
        However that would clearly go way beyond its purpose and scope as being primarily a test bed for the portlet container itself.
        The Pluto Portal Driver never was intended nor scoped as a production quality portal solution.
        Production quality features like parallel request processing (and many others) however can be found in real portal solutions like Jetspeed Portal.

        My conclusion is that the multi-threaded event delivery as implemented in the EventProcessingServiceImpl of the Pluto Portal Driver is fundamentally unreliable and beyond the intended scope and purpose.
        I will therefore disable this "feature" and replace it with a plain in-process event delivery solution by copying the logic from the PortletWindowThread.run() method inline in the EventProcessingServiceImpl.

        Show
        Ate Douma added a comment - Hi Mark, You assessment is spot on, but effectively you're patch trims down the multi-threaded event delivery into a single synchronised event handling, that is: for the most/critical part (the actual invocation of the portlet). However, synchronising only this part still won't make it a 100% bullet proof solution as even in the pre/post processing of the event handling it is possible to encounter threads overwriting part of others state, especially the request attributes. The real cause of this is that the Pluto Portal Driver doesn't properly manage request states per portlet window itself, but simplistically stores all request state (attributes) within the single underlying client HttpServletRequest. Please note: this is not an issue of the Pluto container implementation itself which is completely (request) stateless, which was one of the most critical goals of the Pluto 2.0 container redesign. FYI: in Jetspeed Portal 2.x we actively leverage this capability of the Pluto container for real thread-save parallel request processing and in Jetspeed we maintain the request state for each portlet window completely isolated from the underlying HttpServletRequest. Providing equally thread-save parallel request processing to the Pluto Portal Driver would require very extensive enhancements and redesign. However that would clearly go way beyond its purpose and scope as being primarily a test bed for the portlet container itself. The Pluto Portal Driver never was intended nor scoped as a production quality portal solution. Production quality features like parallel request processing (and many others) however can be found in real portal solutions like Jetspeed Portal. My conclusion is that the multi-threaded event delivery as implemented in the EventProcessingServiceImpl of the Pluto Portal Driver is fundamentally unreliable and beyond the intended scope and purpose. I will therefore disable this "feature" and replace it with a plain in-process event delivery solution by copying the logic from the PortletWindowThread.run() method inline in the EventProcessingServiceImpl.
        Hide
        Mark Piper added a comment -

        Zip file containing test portlets and a test case - this hits a page called "mark", which needs to be set up beforehand with the 3 portlets present on it. The test case simply loops, and hopefully you'll see the NPE's appear every now and again in the Pluto logs.

        Also attached is our patch.

        Show
        Mark Piper added a comment - Zip file containing test portlets and a test case - this hits a page called "mark", which needs to be set up beforehand with the 3 portlets present on it. The test case simply loops, and hopefully you'll see the NPE's appear every now and again in the Pluto logs. Also attached is our patch.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development