ICEfaces
  1. ICEfaces
  2. ICE-9447

ConcurrentModificationException in InputRichTextResourceHandler

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: EE-3.0.0.GA_P01, EE-3.2.0.GA
    • Fix Version/s: EE-3.3.0.GA_P01, 4.0.BETA, 4.0
    • Component/s: ICE-Components
    • Labels:
      None
    • Environment:
      All
    • Assignee Priority:
      P1
    • Salesforce Case Reference:

      Description

      A customer has reported the following issue:

      "under high load we get a ConcurrentModificationException from InputRichTextResourceHandler.calculateMappings().

      The most probable reason for this is that the resource handler as a singleton gets called by the multi-threaded PreRenderViewEvent and thus has to synchronize the access to its variables (in this case allResources, but imageResources is dangerous as well)."
      1. stacktrace.txt
        6 kB
        Arran Mccullough

        Activity

        Hide
        Mark Collette added a comment -

        Any ResourceHandler would be concurrently accessed by different user's lifecycles, so should hold all state either in a method's scope, or if state needs to transcend lifecycles, then hold it in the appropriate Flash/FacesContext/Session/Application map.

        Show
        Mark Collette added a comment - Any ResourceHandler would be concurrently accessed by different user's lifecycles, so should hold all state either in a method's scope, or if state needs to transcend lifecycles, then hold it in the appropriate Flash/FacesContext/Session/Application map.
        Hide
        Arran Mccullough added a comment -

        Attached full error stack trace

        Show
        Arran Mccullough added a comment - Attached full error stack trace
        Hide
        Deryk Sinotte added a comment -

        Just adding my comments from the email thread:

        ResourceHandlers need to be thread-safe (as per the JavaDoc) as I believe there is only one instance of each handler per application so multiple threads could potentially get in there.

        The problem seems to be that, inside the constructor of our InputRichTextResourceHandler, we register to listen for PreRenderViewEvent. Multiple clients can trigger this handler so multiple events could be arriving at any given time (for each view about to rendered). Inside our handler for this event, we take some of our collections (allResources, imageResources, cssResources) and calculate the mappings during the first request. However, neither the methods nor the collections themselves are thread-safe and during calculation of the mappings we iterate over all of them.

        Since these mappings don't need to be calculated more than once (keeping portlets in mind), then, as Mark noted above, maybe we should either be storing the collections (or just a flag) in the appropriate scope indicating that the calculations have been done.

        Show
        Deryk Sinotte added a comment - Just adding my comments from the email thread: ResourceHandlers need to be thread-safe (as per the JavaDoc) as I believe there is only one instance of each handler per application so multiple threads could potentially get in there. The problem seems to be that, inside the constructor of our InputRichTextResourceHandler, we register to listen for PreRenderViewEvent. Multiple clients can trigger this handler so multiple events could be arriving at any given time (for each view about to rendered). Inside our handler for this event, we take some of our collections (allResources, imageResources, cssResources) and calculate the mappings during the first request. However, neither the methods nor the collections themselves are thread-safe and during calculation of the mappings we iterate over all of them. Since these mappings don't need to be calculated more than once (keeping portlets in mind), then, as Mark noted above, maybe we should either be storing the collections (or just a flag) in the appropriate scope indicating that the calculations have been done.
        Hide
        Arturo Zambrano added a comment -

        The first effort consists in modifying the resource handling process, so that the resources don't have to be mapped dynamically while the application is running. The resources are now being mapped statically, using #resource expressions, so that the server only has to convert such expressions to actual URLs, without having to calculate the mappings at each request. This is the same idea of ICE-8715 with ace:richTextEntry.

        Show
        Arturo Zambrano added a comment - The first effort consists in modifying the resource handling process, so that the resources don't have to be mapped dynamically while the application is running. The resources are now being mapped statically, using #resource expressions, so that the server only has to convert such expressions to actual URLs, without having to calculate the mappings at each request. This is the same idea of ICE-8715 with ace:richTextEntry.
        Hide
        Arturo Zambrano added a comment -

        Committed new approach to serve CKEditor resources at revision 37245 in the trunk and at revision 37246 in the p01 tag.

        Now, resource mappings are not calculated dynamically when the app is deployed. Instead, there's a static ckeditor.mappings.js resource that contains resource expressions, which the new resource handler interprets in the same way as with CSS resources. The resource handler class now doesn't keep much state: only a reference to the wrapped object and a reference to the resource object.

        Show
        Arturo Zambrano added a comment - Committed new approach to serve CKEditor resources at revision 37245 in the trunk and at revision 37246 in the p01 tag. Now, resource mappings are not calculated dynamically when the app is deployed. Instead, there's a static ckeditor.mappings.js resource that contains resource expressions, which the new resource handler interprets in the same way as with CSS resources. The resource handler class now doesn't keep much state: only a reference to the wrapped object and a reference to the resource object.
        Hide
        Arturo Zambrano added a comment - - edited

        With the new approach, there are no collections or other objects that could potentially be concurrently modified by different threads, so that alone prevents concurrent modification exceptions. The only instance variable of the resource handler is the resource object itself, which is simply registered at the first request, without any further processing. Once this resource is registered, the instance variable is not null, and future threads will check for null and avoid re-registering this resource. Just to make things more robust, the line where this instance variable is checked for null was enclosed in a synchronized statement, in order to completely prevent registering this resource multiple times under extreme circumstances.

        This was committed at revision 37247 to the trunk and at revision 37248 to the p01 tag.

        Show
        Arturo Zambrano added a comment - - edited With the new approach, there are no collections or other objects that could potentially be concurrently modified by different threads, so that alone prevents concurrent modification exceptions. The only instance variable of the resource handler is the resource object itself, which is simply registered at the first request, without any further processing. Once this resource is registered, the instance variable is not null, and future threads will check for null and avoid re-registering this resource. Just to make things more robust, the line where this instance variable is checked for null was enclosed in a synchronized statement, in order to completely prevent registering this resource multiple times under extreme circumstances. This was committed at revision 37247 to the trunk and at revision 37248 to the p01 tag.
        Hide
        Florian Jung added a comment -

        Is this patch already included in the current snapshot for version 3.4? I am asking because I still see this exception from time to time when starting parallel selenium tests.

        Show
        Florian Jung added a comment - Is this patch already included in the current snapshot for version 3.4? I am asking because I still see this exception from time to time when starting parallel selenium tests.
        Hide
        Mark Collette added a comment -

        Please attach an exception stack trace.

        Show
        Mark Collette added a comment - Please attach an exception stack trace.
        Hide
        Florian Jung added a comment -
        WARN - org.icefaces.impl.application.ExtendedExceptionHandler:80 - queued exception
        java.util.ConcurrentModificationException
         at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:819)
         at java.util.ArrayList$Itr.next(ArrayList.java:791)
         at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler.calculateMappings(InputRichTextResourceHandler.java:148)
         at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler.access$300(InputRichTextResourceHandler.java:37)
         at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler$1.processEvent(InputRichTextResourceHandler.java:78)
         at javax.faces.event.SystemEvent.processListener(SystemEvent.java:108)
         at javax.faces.event.ComponentSystemEvent.processListener(ComponentSystemEvent.java:118)
         at com.sun.faces.application.ApplicationImpl.processListeners(ApplicationImpl.java:2187)
         at com.sun.faces.application.ApplicationImpl.invokeListenersFor(ApplicationImpl.java:2163)
         at com.sun.faces.application.ApplicationImpl.publishEvent(ApplicationImpl.java:303)
         at com.sun.faces.application.ApplicationImpl.publishEvent(ApplicationImpl.java:247)
         at org.jboss.weld.environment.servlet.jsf.ForwardingApplication.publishEvent(ForwardingApplication.java:287)
         at com.sun.faces.lifecycle.RenderResponsePhase.execute(RenderResponsePhase.java:107)
         at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:101)
         at com.sun.faces.lifecycle.LifecycleImpl.render(LifecycleImpl.java:219)
         at javax.faces.webapp.FacesServlet.service(FacesServlet.java:647)
         at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305)
         at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
         at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222)
         at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123)
         at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:472)
         at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171)
         at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:99)
         at org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953)
         at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
         at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:408)
         at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1008)
         at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:589)
         at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:312)
         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
         at java.lang.Thread.run(Thread.java:722)
        
        Show
        Florian Jung added a comment - WARN - org.icefaces.impl.application.ExtendedExceptionHandler:80 - queued exception java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:819) at java.util.ArrayList$Itr.next(ArrayList.java:791) at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler.calculateMappings(InputRichTextResourceHandler.java:148) at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler.access$300(InputRichTextResourceHandler.java:37) at com.icesoft.faces.component.inputrichtext.InputRichTextResourceHandler$1.processEvent(InputRichTextResourceHandler.java:78) at javax.faces.event.SystemEvent.processListener(SystemEvent.java:108) at javax.faces.event.ComponentSystemEvent.processListener(ComponentSystemEvent.java:118) at com.sun.faces.application.ApplicationImpl.processListeners(ApplicationImpl.java:2187) at com.sun.faces.application.ApplicationImpl.invokeListenersFor(ApplicationImpl.java:2163) at com.sun.faces.application.ApplicationImpl.publishEvent(ApplicationImpl.java:303) at com.sun.faces.application.ApplicationImpl.publishEvent(ApplicationImpl.java:247) at org.jboss.weld.environment.servlet.jsf.ForwardingApplication.publishEvent(ForwardingApplication.java:287) at com.sun.faces.lifecycle.RenderResponsePhase.execute(RenderResponsePhase.java:107) at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:101) at com.sun.faces.lifecycle.LifecycleImpl.render(LifecycleImpl.java:219) at javax.faces.webapp.FacesServlet.service(FacesServlet.java:647) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:305) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:222) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:123) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:472) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:171) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:99) at org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:953) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:408) at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1008) at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:589) at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:312) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) at java.lang.Thread.run(Thread.java:722)
        Hide
        Mark Collette added a comment -

        Thank you, that does look like this specific issue. Are you running code from after subversion 37247? That's the revision where this fix should be complete.

        Show
        Mark Collette added a comment - Thank you, that does look like this specific issue. Are you running code from after subversion 37247? That's the revision where this fix should be complete.
        Hide
        Florian Jung added a comment - - edited

        I am using the current snapshot version (compat-3.4.0-20130723.214239-4 and the other artifacts build from this day respectivelly) that is available at http://anonsvn.icesoft.org/repo/maven2/snapshots/org/icefaces/. Unfortunately I have no clue, if revision 37247 has been incorporated into the build, but from the heading when surfing this URL it seems to me as if this is the right build.

        Show
        Florian Jung added a comment - - edited I am using the current snapshot version (compat-3.4.0-20130723.214239-4 and the other artifacts build from this day respectivelly) that is available at http://anonsvn.icesoft.org/repo/maven2/snapshots/org/icefaces/ . Unfortunately I have no clue, if revision 37247 has been incorporated into the build, but from the heading when surfing this URL it seems to me as if this is the right build.
        Hide
        Mark Collette added a comment -

        It looks like the version you have is from July 23rd, and the fix was complete on July 26, so no I don't believe you have the fix.

        Show
        Mark Collette added a comment - It looks like the version you have is from July 23rd, and the fix was complete on July 26, so no I don't believe you have the fix.
        Hide
        Florian Jung added a comment -

        That would explain the exception - thanks for checking and sorry for the inconvenience. Isn't building the snapshot version triggered by commits on a - let's say daily or weekly basis? So I am curious, when the patched version will be available in the repo. Anyway - great work guys. Keep on...

        Show
        Florian Jung added a comment - That would explain the exception - thanks for checking and sorry for the inconvenience. Isn't building the snapshot version triggered by commits on a - let's say daily or weekly basis? So I am curious, when the patched version will be available in the repo. Anyway - great work guys. Keep on...

          People

          • Assignee:
            Arturo Zambrano
            Reporter:
            Arran Mccullough
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: