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

        Arran Mccullough created issue -
        Ken Fyten made changes -
        Field Original Value New Value
        Assignee Arturo Zambrano [ artzambrano ]
        Fix Version/s EE-3.3.0.GA_P01 [ 11174 ]
        Fix Version/s 3.4 [ 10770 ]
        Assignee Priority P1 [ 10010 ]
        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.
        Arran Mccullough made changes -
        Salesforce Case Reference 5007000000Uc3vdAAB
        Hide
        Arran Mccullough added a comment -

        Attached full error stack trace

        Show
        Arran Mccullough added a comment - Attached full error stack trace
        Arran Mccullough made changes -
        Attachment stacktrace.txt [ 16138 ]
        Arran Mccullough made changes -
        Issue Type Improvement [ 4 ] Bug [ 1 ]
        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.
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #37237 Fri Jul 26 09:52:42 MDT 2013 art.zambrano ICE-9447 added ststic ckeditor.mapping.js with resource expressions and modified CSS files to use resource expressions; modified resource handler to serve the static ckeditor.mapping.js resource, without having to calculate the mappings every time an application is deployed
        Files Changed
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/v2/templates.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/v2/dialog.css
        Commit graph ADD /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/ckeditor.mapping.js
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/kama/editor.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/plugins/scayt/dialogs/toolbar.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/kama/dialog.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/inputrichtext/InputRichTextResourceHandler.java
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/contents.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/plugins/uicolor/yui/assets/yui.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/kama/templates.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/plugins/wsc/dialogs/wsc.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/v2/editor.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/office2003/templates.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/office2003/editor.css
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/skins/office2003/dialog.css
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #37242 Fri Jul 26 10:34:51 MDT 2013 art.zambrano ICE-9447 applied fix ICE-8715, updated readme.txt
        Files Changed
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/readme.txt
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/resources/META-INF/resources/inputrichtext/ckeditor/ckeditor.mapping.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #37245 Fri Jul 26 10:41:06 MDT 2013 art.zambrano ICE-9447 removed build script to generate list of CKEditor resources
        Files Changed
        Commit graph DEL /icefaces3/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/inputrichtext/build.xml
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/build.xml
        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.
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #37247 Fri Jul 26 11:19:24 MDT 2013 art.zambrano ICE-9447 enclosed starting point of resource creation in a synchronized statement
        Files Changed
        Commit graph MODIFY /icefaces3/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/inputrichtext/InputRichTextResourceHandler.java
        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.
        Arturo Zambrano made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        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...
        Ken Fyten made changes -
        Fix Version/s 4.0 [ 11382 ]
        Ken Fyten made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved: