Jetspeed 2
  1. Jetspeed 2
  2. JS2-226

Page Aggregation using STRATEGY_PARALLEL severly broken

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Aggregation
    • Labels:
      None
    • Environment:
      Tomcat 5.0.28

      Description

      While trying to evaluate the status of JS2-17 I tested PageAggregation with PageAggregatorImpl.STRATEGY_PARALLEL and the effect is a total breakdown!
      I'll look into this AFTER the M2 release.
      Not sure if the current problem is related to JS2-17 but I'll check that too.

      1. JS2-226-diff.txt
        33 kB
        Woonsan Ko
      2. more-fail-safe-diff.txt
        21 kB
        Woonsan Ko

        Activity

        Ate Douma created issue -
        Hide
        Jian Liao added a comment -

        I am interested in this bug, too. It seems that all the portlet instances share the same HttpServletRequest object underlying, which is the original one. So their attributes HashMap must be the same. That is the reason for thread competition.

        I also saw that class "org.apache.jetspeed.engine.servlet.ServletRequestImpl" is trying to fix this bug by encode the attribute name to avoid conflict.

        I am wondering that why just encode the attributes which start with "org.apache.jetspeed". why don't override the removeAttribute method, too. Currently removeAttribute() operation is meanless, the default removeAttribute method can not just take the simple attribute name to remove attribute without encode it.

        Does this class finish?

        Show
        Jian Liao added a comment - I am interested in this bug, too. It seems that all the portlet instances share the same HttpServletRequest object underlying, which is the original one. So their attributes HashMap must be the same. That is the reason for thread competition. I also saw that class "org.apache.jetspeed.engine.servlet.ServletRequestImpl" is trying to fix this bug by encode the attribute name to avoid conflict. I am wondering that why just encode the attributes which start with "org.apache.jetspeed". why don't override the removeAttribute method, too. Currently removeAttribute() operation is meanless, the default removeAttribute method can not just take the simple attribute name to remove attribute without encode it. Does this class finish?
        Hide
        Ate Douma added a comment -

        The problem is bigger (and deeper) than just the sharing of the same HttpServletRequest.
        If that was the case I would have been able to fix this long time ago as I already have done so in my test environment.

        But, the real problem is the Tomcat engine: its not build for thread save processing of a request.
        At the core of a HttpServletRequest is a Tomcat CoyoteRequest. You can't fake, wrap, or foul that one.

        Right now, I'm quite pessimistic about getting this issue fixed (for Tomcat at least).

        Show
        Ate Douma added a comment - The problem is bigger (and deeper) than just the sharing of the same HttpServletRequest. If that was the case I would have been able to fix this long time ago as I already have done so in my test environment. But, the real problem is the Tomcat engine: its not build for thread save processing of a request. At the core of a HttpServletRequest is a Tomcat CoyoteRequest. You can't fake, wrap, or foul that one. Right now, I'm quite pessimistic about getting this issue fixed (for Tomcat at least).
        Hide
        Ate Douma added a comment -

        I'm going to reassign this issue to David Sean Taylor as he might be able to indicate if his current enhancements can resolve this somehow.
        I leave it to him to decide if this can be resolved (if at all) for 2.0-FINAL.

        Show
        Ate Douma added a comment - I'm going to reassign this issue to David Sean Taylor as he might be able to indicate if his current enhancements can resolve this somehow. I leave it to him to decide if this can be resolved (if at all) for 2.0-FINAL.
        Ate Douma made changes -
        Field Original Value New Value
        Assignee Ate Douma [ adouma ] David Sean Taylor [ taylor ]
        Hide
        David Sean Taylor added a comment -

        Going to set this to post 2.0
        Ive had some luck with Velocity-only portlets. JSP was crashing on me right and left.
        Will be revisiting this post 2.0 along with caching (finally)

        Show
        David Sean Taylor added a comment - Going to set this to post 2.0 Ive had some luck with Velocity-only portlets. JSP was crashing on me right and left. Will be revisiting this post 2.0 along with caching (finally)
        David Sean Taylor made changes -
        Affects Version/s 2.0-M2 [ 11015 ]
        Affects Version/s 2.0-dev/cvs [ 10598 ]
        Description While trying to evaluate the status of JS2-17 I tested PageAggregation with PageAggregatorImpl.STRATEGY_PARALLEL and the effect is a total breakdown!
        I'll look into this AFTER the M2 release.
        Not sure if the current problem is related to JS2-17 but I'll check that too.
        While trying to evaluate the status of JS2-17 I tested PageAggregation with PageAggregatorImpl.STRATEGY_PARALLEL and the effect is a total breakdown!
        I'll look into this AFTER the M2 release.
        Not sure if the current problem is related to JS2-17 but I'll check that too.
        Affects Version/s 2.0-POST [ 12310617 ]
        Hide
        Woonsan Ko added a comment -

        I've investigated this issue for last two months to provide parallel processing feature for my client.

        During my work, I found the following: (I feel that this issue is so big and deep, as Ate said.)

        • The request of tomcat is not thread-safe, but anyway the last wrapper is org.apache.jetspeed.engine.servlet.ServletRequestImpl for portlets. (So the implementation can reference other sources, such as current thread.)
        • Portlet-level attributes are encoded in JS2, but upper-level ones are not. e.g) portlet instance, portlet config, portlet request, portlet response, method id, portlet name and context.
        • On the other hand, in parallel processing, there is a thread pool. (WorkerMonitor and Worker)
        • Therefore, I thought that some upper-level attributes (such as "javax.portlet.request") could be stored in the worker thread, not just in request, and then in parallel processing the attributes could be referenced via the worker thread, not just via request. (Actually, I implemented the java.util.Map interface in my test worker class.)

        I've been experimented the idea by writing some messy codes which extended or overrode the existing components, and it seems like working well now in point of functionality
        However, the performance is not as good as I expected in my test environment. (One CPU PC.)

        I'm not sure the parallel processing is better in point of performance (Response time).

        Anyway, I want to know how you think of my idea. (Some attributes can be stored and referenced in the worker thread for the parallel rendering portlets.) Any comment welcomed.

        Show
        Woonsan Ko added a comment - I've investigated this issue for last two months to provide parallel processing feature for my client. During my work, I found the following: (I feel that this issue is so big and deep, as Ate said.) The request of tomcat is not thread-safe, but anyway the last wrapper is org.apache.jetspeed.engine.servlet.ServletRequestImpl for portlets. (So the implementation can reference other sources, such as current thread.) Portlet-level attributes are encoded in JS2, but upper-level ones are not. e.g) portlet instance, portlet config, portlet request, portlet response, method id, portlet name and context. On the other hand, in parallel processing, there is a thread pool. (WorkerMonitor and Worker) Therefore, I thought that some upper-level attributes (such as "javax.portlet.request") could be stored in the worker thread, not just in request, and then in parallel processing the attributes could be referenced via the worker thread, not just via request. (Actually, I implemented the java.util.Map interface in my test worker class.) I've been experimented the idea by writing some messy codes which extended or overrode the existing components, and it seems like working well now in point of functionality However, the performance is not as good as I expected in my test environment. (One CPU PC.) I'm not sure the parallel processing is better in point of performance (Response time). Anyway, I want to know how you think of my idea. (Some attributes can be stored and referenced in the worker thread for the parallel rendering portlets.) Any comment welcomed.
        Hide
        David Sean Taylor added a comment -

        Hi Woonsan,

        I was planning on working on the parallel strategy in September for the 2.1 release. My schedule has slipped a little though, so Im encouraged to hear someone else show interest in the parallel strategy.

        I wouldn't be put off by the performance.
        There are some very valuable use cases that make the parallel strategy valuable. If all of your portlets are simply retrieving small snippets of content, and displaying them, there will be no advantage. In fact, the overhead will make the overall aggregation slower. I would think that we would want to make use of hints to help with the parallel aggregation process, such as statistical analysis and configuration. With configuration, a portlet can be marked as requiring parallel. With statistics, we can keep track of which portlets are slow, and move those into thread groups based on their historical performance.

        The key use case for parallel rendering is when one or more portlets are taking a lot longer than other portlets to render, then the other portlets will not have to wait on them as they do in the sequential strategy. Additionally, we can put a ms threshold on all portlet threads returning, so that if a portlet takes more than say 4000 ms, we override the content with "Portlet Not Available" and return the entire response stream. The thread will have to be collected later.

        I think your idea, of storing attributes in the worker, is sound.
        In my testing, I also discovered that Tomcat requests are not thread-safe.

        We are actually solving this problem from two different approaches in 2.1. Parallel rendering on the server is on approach. The second approach is parallel rendering on the client, with the Jetspeed Desktop.
        The Desktop solution distributes the aggregation process onto clients.

        Anyway, I'd be very happy to work with you on this.

        Show
        David Sean Taylor added a comment - Hi Woonsan, I was planning on working on the parallel strategy in September for the 2.1 release. My schedule has slipped a little though, so Im encouraged to hear someone else show interest in the parallel strategy. I wouldn't be put off by the performance. There are some very valuable use cases that make the parallel strategy valuable. If all of your portlets are simply retrieving small snippets of content, and displaying them, there will be no advantage. In fact, the overhead will make the overall aggregation slower. I would think that we would want to make use of hints to help with the parallel aggregation process, such as statistical analysis and configuration. With configuration, a portlet can be marked as requiring parallel. With statistics, we can keep track of which portlets are slow, and move those into thread groups based on their historical performance. The key use case for parallel rendering is when one or more portlets are taking a lot longer than other portlets to render, then the other portlets will not have to wait on them as they do in the sequential strategy. Additionally, we can put a ms threshold on all portlet threads returning, so that if a portlet takes more than say 4000 ms, we override the content with "Portlet Not Available" and return the entire response stream. The thread will have to be collected later. I think your idea, of storing attributes in the worker, is sound. In my testing, I also discovered that Tomcat requests are not thread-safe. We are actually solving this problem from two different approaches in 2.1. Parallel rendering on the server is on approach. The second approach is parallel rendering on the client, with the Jetspeed Desktop. The Desktop solution distributes the aggregation process onto clients. Anyway, I'd be very happy to work with you on this.
        Hide
        Woonsan Ko added a comment -

        David,

        Thanks for your comment. I'm very happy with your insights.
        Very important clues from you:

        • Defered rendering for late-rendering portlets is possible. (Worker can store the late content for later use.)
        • Portlets can be marked as parallel processing.
        • Jetspeed Desktop can be a good alternative.

        On the other hand, I think my idea has some problems like the following:

        • If some portlets are parallel and otheres are not, then the current threads are not worker thread only. If a current thread is one of the servlet container, then we cannot store attributes in that. I need to have a solution for this.

        Also I think:

        • Defered rendering has no problem. I think the aggregator can do that according to portlet definitions.
        • I think Jetspeed Desktop is one of the best solutions for PC-based browser clients. However, the server-side solution is meaningful, as David said.

        By the way, I have a question. There are two classes, PageAggregatorImpl and AsyncPageAggregatorImpl. Which is proper, PageAggregatorImpl with parallel option or AsyncPageAggregatorImpl?
        I guess PageAggregatorImpl is more adequate because it has to support portlets marked as parallel. Am I right?

        I will post my test patches when I finish cleaning my messy codes. (about until September 8.)
        Thanks again, David.

        Show
        Woonsan Ko added a comment - David, Thanks for your comment. I'm very happy with your insights. Very important clues from you: Defered rendering for late-rendering portlets is possible. (Worker can store the late content for later use.) Portlets can be marked as parallel processing. Jetspeed Desktop can be a good alternative. On the other hand, I think my idea has some problems like the following: If some portlets are parallel and otheres are not, then the current threads are not worker thread only. If a current thread is one of the servlet container, then we cannot store attributes in that. I need to have a solution for this. Also I think: Defered rendering has no problem. I think the aggregator can do that according to portlet definitions. I think Jetspeed Desktop is one of the best solutions for PC-based browser clients. However, the server-side solution is meaningful, as David said. By the way, I have a question. There are two classes, PageAggregatorImpl and AsyncPageAggregatorImpl. Which is proper, PageAggregatorImpl with parallel option or AsyncPageAggregatorImpl? I guess PageAggregatorImpl is more adequate because it has to support portlets marked as parallel. Am I right? I will post my test patches when I finish cleaning my messy codes. (about until September 8.) Thanks again, David.
        Hide
        David Sean Taylor added a comment -

        > On the other hand, I think my idea has some problems like the following:
        > - If some portlets are parallel and otheres are not, then the current threads are not worker thread only. If a current thread is one of the servlet
        > container, then we cannot store attributes in that. I need to have a solution for this.

        We will need to start off a single thread that executes all sequential tasks

        > Which is proper, PageAggregatorImpl with parallel option or AsyncPageAggregatorImpl?

        i've been working on the AsyncPageAggregatorImpl, just to keep things separated during prototypes

        Show
        David Sean Taylor added a comment - > On the other hand, I think my idea has some problems like the following: > - If some portlets are parallel and otheres are not, then the current threads are not worker thread only. If a current thread is one of the servlet > container, then we cannot store attributes in that. I need to have a solution for this. We will need to start off a single thread that executes all sequential tasks > Which is proper, PageAggregatorImpl with parallel option or AsyncPageAggregatorImpl? i've been working on the AsyncPageAggregatorImpl, just to keep things separated during prototypes
        Hide
        Woonsan Ko added a comment -

        Hi David,

        My test patches are posted, which make parallel rendering just work.
        My patches do not contain timeout, defered rendering, etc.
        Here is explanation for my patches.

        [Basic idea]

        • Because the request is not thread-safe, in parallel mode, our request wrapper needs to store attributes in the worker thread. The worker thread can implement java.util.Map interface to allow manipulating attributes.
        • Other thread-unsafe codes can look up attributes from the current worker in parallel mode.

        [Changes by me]

        • jetspeed-api/.../PortalReservedParameters.java
        • A constant for portlet definition attribute added.
        • jetspeed-api/.../PortletRenderer.java
        • The render method now returns RenderingJob instance, which will be used for portlets rendering synchronization.
        • jetspeed-api/.../RenderingJob.java
        • PortletContent getPortletContent() is added. In portlets rendering synchronization in AsyncPageAggregatorImpl, portlet content needs to be waited until completion.
        • components/portal/.../AsyncPageAggregatorImpl.java
        • I rewrote portlets rendering synchronization.
        • components/portal/.../PortletRendererImpl.java
        • In parallel mode, it stores attributes into rendering job object, and the rendering job will pass the attributes to worker when it runs.
        • components/portal/.../WorkerImpl.java
        • It contains attributes map and implements java.util.Map to store and allow to manipulate attributes.
        • components/portal/.../RenderingJobImpl.java
        • It passes attributes to the current running worker
        • And it stores portlet definition attribute to reset wrong portlet entities.
        • components/portal/.../ServletPortletInvoker.java
        • This class was not thread-safe. It has a member variable for portlet definition.
        • So, in parallel mode, portlet definition is looked up from worker now.
        • Also it stores other attributes into worker in parallel mode.
        • components/portal/.../ServletRequestImpl.java
        • This wrapper class can look up and store attributes from worker or from servlet request.
        • commons/.../JetspeedContainerServlet.java
        • This servlet was not thread-safe because it looked up portlet information from the servlet request.
        • So, this servlet can look up those from worker now.
        • components/portal/.../JetspeedRequestContextComponent.java
        • This component looked up principal from RequestContextComponent, which looked up RequestContext from ThreadLocal member. However, in parallel mode, the ThreadLocal member is null. So, in parallel mode, it looks up that from current running worker.

        [Problems remained]

        • Sometimes, some portlet cannot render when page is visited for the first time after tomcat starts up. After that, this problem does not occur any more. You can find the error message in the tomcat log file. (NoClassDefFoundError...)
        • The portlets in JSF Demo page do not work properly. Sometimes one of portlets cannot render with exception. Probably, some attributes are needed for JSF portlets.
        • In parallel mode, portlet definition of portlet entity is replaced with wrong object, but I could not find the reason. So ServletPortletInvoker checks if the portlet definition is wrong and resets.

        Thanks for reading.
        Any comment welcomed.

        Show
        Woonsan Ko added a comment - Hi David, My test patches are posted, which make parallel rendering just work. My patches do not contain timeout, defered rendering, etc. Here is explanation for my patches. [Basic idea] Because the request is not thread-safe, in parallel mode, our request wrapper needs to store attributes in the worker thread. The worker thread can implement java.util.Map interface to allow manipulating attributes. Other thread-unsafe codes can look up attributes from the current worker in parallel mode. [Changes by me] jetspeed-api/.../PortalReservedParameters.java A constant for portlet definition attribute added. jetspeed-api/.../PortletRenderer.java The render method now returns RenderingJob instance, which will be used for portlets rendering synchronization. jetspeed-api/.../RenderingJob.java PortletContent getPortletContent() is added. In portlets rendering synchronization in AsyncPageAggregatorImpl, portlet content needs to be waited until completion. components/portal/.../AsyncPageAggregatorImpl.java I rewrote portlets rendering synchronization. components/portal/.../PortletRendererImpl.java In parallel mode, it stores attributes into rendering job object, and the rendering job will pass the attributes to worker when it runs. components/portal/.../WorkerImpl.java It contains attributes map and implements java.util.Map to store and allow to manipulate attributes. components/portal/.../RenderingJobImpl.java It passes attributes to the current running worker And it stores portlet definition attribute to reset wrong portlet entities. components/portal/.../ServletPortletInvoker.java This class was not thread-safe. It has a member variable for portlet definition. So, in parallel mode, portlet definition is looked up from worker now. Also it stores other attributes into worker in parallel mode. components/portal/.../ServletRequestImpl.java This wrapper class can look up and store attributes from worker or from servlet request. commons/.../JetspeedContainerServlet.java This servlet was not thread-safe because it looked up portlet information from the servlet request. So, this servlet can look up those from worker now. components/portal/.../JetspeedRequestContextComponent.java This component looked up principal from RequestContextComponent, which looked up RequestContext from ThreadLocal member. However, in parallel mode, the ThreadLocal member is null. So, in parallel mode, it looks up that from current running worker. [Problems remained] Sometimes, some portlet cannot render when page is visited for the first time after tomcat starts up. After that, this problem does not occur any more. You can find the error message in the tomcat log file. (NoClassDefFoundError...) The portlets in JSF Demo page do not work properly. Sometimes one of portlets cannot render with exception. Probably, some attributes are needed for JSF portlets. In parallel mode, portlet definition of portlet entity is replaced with wrong object, but I could not find the reason. So ServletPortletInvoker checks if the portlet definition is wrong and resets. Thanks for reading. Any comment welcomed.
        Woonsan Ko made changes -
        Attachment JS2-226-diff.txt [ 12340359 ]
        David Sean Taylor made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        David Sean Taylor added a comment -

        Patch applied and tested, thanks Woonsan.
        I will send you a list of remaining tasks and possible schedule for implementing these tasks between us
        Note:
        We test for one of our worker threads by testing to see if the current thread implements the Map interface
        Im just wondering if this will lead to problems, say if some app server decides to also implement the Map interface on their thread. We might want to have a more fail-safe method of testing for the current thread being one of our boys

        Show
        David Sean Taylor added a comment - Patch applied and tested, thanks Woonsan. I will send you a list of remaining tasks and possible schedule for implementing these tasks between us Note: We test for one of our worker threads by testing to see if the current thread implements the Map interface Im just wondering if this will lead to problems, say if some app server decides to also implement the Map interface on their thread. We might want to have a more fail-safe method of testing for the current thread being one of our boys
        Hide
        Woonsan Ko added a comment -

        You are right, David. Thanks.

        I modified some sources, so they are testing to see if the current thread implements 'org.apache.jetspeed.aggregator.Worker' interface, not 'java.util.Map' interface.

        Also, current thread context attributes are retrieved via 'org.apache.jetspeed.aggregator.CurrentWorkerContext' using 'ThreadLocal', not 'Worker' itself. So I removed Map implementation from 'WorkerImpl'.

        FYI, when I added a new class named 'CurrentWorkerContext', I refered to 'org.apache.jetspeed.i18n.CurrentLocale' class in 'jetspeed-api'.

        I did some basic tests and it worked as it did.

        Show
        Woonsan Ko added a comment - You are right, David. Thanks. I modified some sources, so they are testing to see if the current thread implements 'org.apache.jetspeed.aggregator.Worker' interface, not 'java.util.Map' interface. Also, current thread context attributes are retrieved via 'org.apache.jetspeed.aggregator.CurrentWorkerContext' using 'ThreadLocal', not 'Worker' itself. So I removed Map implementation from 'WorkerImpl'. FYI, when I added a new class named 'CurrentWorkerContext', I refered to 'org.apache.jetspeed.i18n.CurrentLocale' class in 'jetspeed-api'. I did some basic tests and it worked as it did.
        Woonsan Ko made changes -
        Attachment more-fail-safe-diff.txt [ 12346743 ]
        Hide
        David Sean Taylor added a comment -

        the base development is completed for version 2.1

        Show
        David Sean Taylor added a comment - the base development is completed for version 2.1
        David Sean Taylor made changes -
        Fix Version/s 2.1 [ 12310617 ]
        Resolution Fixed [ 1 ]
        Status In Progress [ 3 ] Resolved [ 5 ]
        Ate Douma made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            David Sean Taylor
            Reporter:
            Ate Douma
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development