Commons Digester
  1. Commons Digester
  2. DIGESTER-163

ConcurrentModificationException creating a new Digester via loaderInstance.newDigester()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Linux, JDK 6

      Description

      I am gettig a ConcurrentModificationException when trying to create new Digester instance from a configured loader:
      Trace is:

      java.util.ConcurrentModificationException: null
      	at java.util.LinkedList$ListItr.checkForComodification(LinkedList.java:761) ~[na:1.6.0_27]
      	at java.util.LinkedList$ListItr.next(LinkedList.java:696) ~[na:1.6.0_27]
      	at org.apache.commons.digester3.binder.FromBinderRuleSet.addRuleInstances(FromBinderRuleSet.java:130) ~[commons-digester3-3.2.jar:3.2]
      	at org.apache.commons.digester3.binder.DigesterLoader.addRules(DigesterLoader.java:581) ~[commons-digester3-3.2.jar:3.2]
      	at org.apache.commons.digester3.binder.DigesterLoader.newDigester(DigesterLoader.java:568) ~[commons-digester3-3.2.jar:3.2]
      	at org.apache.commons.digester3.binder.DigesterLoader.newDigester(DigesterLoader.java:516) ~[commons-digester3-3.2.jar:3.2]
      	at org.apache.commons.digester3.binder.DigesterLoader.newDigester(DigesterLoader.java:475) ~[commons-digester3-3.2.jar:3.2]
      	at org.apache.commons.digester3.binder.DigesterLoader.newDigester(DigesterLoader.java:462) ~[commons-digester3-3.2.jar:3.2]
      

      The binder documentation (employee servlet) and the mailing list did confirm to me, that the loader should be safe to be shared, so this should not happen.

      1. 163.patch
        7 kB
        Torsten Krah
      2. 163-2.patch
        7 kB
        Torsten Krah
      3. cli-mvn-test-withfix.txt
        6 kB
        Torsten Krah
      4. Digester163TestCase.java
        3 kB
        Torsten Krah
      5. stack-afterfix.txt
        23 kB
        Torsten Krah
      6. stack-mvn.txt
        13 kB
        Torsten Krah
      7. stack-next.txt
        14 kB
        Torsten Krah
      8. stack-next2.txt
        14 kB
        Torsten Krah

        Activity

        Hide
        Simone Tripodi added a comment -

        Can you kindly attach a testcase that reproduces the issue? Many thanks in advance!

        Show
        Simone Tripodi added a comment - Can you kindly attach a testcase that reproduces the issue? Many thanks in advance!
        Hide
        Torsten Krah added a comment -

        http://fachschaft.imn.htwk-leipzig.de/~tkrah/digester-163.tar.bz2

        Maven project which does produce some other sax errors too - CME is not yet seen in this test, still searching for ways to provoke - but the other exceptions should not be their either.
        Run mvn test multiple times to see them sometimes.

        Show
        Torsten Krah added a comment - http://fachschaft.imn.htwk-leipzig.de/~tkrah/digester-163.tar.bz2 Maven project which does produce some other sax errors too - CME is not yet seen in this test, still searching for ways to provoke - but the other exceptions should not be their either. Run mvn test multiple times to see them sometimes.
        Hide
        Simone Tripodi added a comment - - edited

        I imported a simplified version of your testcase in the Digester codebase, see r1300873; unfortunately I am not able to reproduce the issue

        IIUC - apologize if I am wrong - the problem could be in your wrappers that allows concurrency in the loader creation - using the ConcurrentHashMap you keep the data structure thread-safe but it doesn't shield from races and concurrent accesses to the Loader creation...

        Can you please check it? TIA!!!

        Show
        Simone Tripodi added a comment - - edited I imported a simplified version of your testcase in the Digester codebase, see r1300873 ; unfortunately I am not able to reproduce the issue IIUC - apologize if I am wrong - the problem could be in your wrappers that allows concurrency in the loader creation - using the ConcurrentHashMap you keep the data structure thread-safe but it doesn't shield from races and concurrent accesses to the Loader creation... Can you please check it? TIA!!!
        Hide
        Torsten Krah added a comment -

        Hm - is it forbidden to create two loaders at different threads? I know that more than one loader may be created, but my calling thread does only got the one who first entered the map.
        Digester.newLoader(...) should be thread-safe at all, i don't know if some other libs i may be using are doing this too - so is there something special with loader creation?

        Show
        Torsten Krah added a comment - Hm - is it forbidden to create two loaders at different threads? I know that more than one loader may be created, but my calling thread does only got the one who first entered the map. Digester.newLoader(...) should be thread-safe at all, i don't know if some other libs i may be using are doing this too - so is there something special with loader creation?
        Hide
        Torsten Krah added a comment -

        Attached modified failing test case for completeness here, although you've got it already via mail.

        Show
        Torsten Krah added a comment - Attached modified failing test case for completeness here, although you've got it already via mail.
        Hide
        Simone Tripodi added a comment -

        two loaders created by different threads have to be allowed, of course, that is why we are investigating about the problem. I am testing your modifications right now, I hope to let you know good news ASAP

        Show
        Simone Tripodi added a comment - two loaders created by different threads have to be allowed, of course, that is why we are investigating about the problem. I am testing your modifications right now, I hope to let you know good news ASAP
        Hide
        Simone Tripodi added a comment -

        Just to keep you updated my tests: I am getting NPE}}s because {{test.xml looks like

        <root>
          <container>
            <header>
              <authors>
                <author>Author 1</author>
              </authors>
            </header>
          </container>
        </root>
        

        while rules are configured like (despite the class renaming)

        <digester-rules>
          <object-create-rule classname="org.apache.commons.digester3.binder.Entity" pattern="container"/>
          <!-- author -->
          <call-method-rule methodname="setAuthor" paramcount="1" pattern="container/header/authors/author"/>
        </digester-rules>
        

        so we have two options:

        • rules pattern are wrong;
        • sample is wrong.

        I am removing the <root> element and see what happens. let you know soon!

        Show
        Simone Tripodi added a comment - Just to keep you updated my tests: I am getting NPE}}s because {{test.xml looks like <root> <container> <header> <authors> <author>Author 1</author> </authors> </header> </container> </root> while rules are configured like (despite the class renaming) <digester-rules> <object-create-rule classname= "org.apache.commons.digester3.binder.Entity" pattern= "container" /> <!-- author --> <call-method-rule methodname= "setAuthor" paramcount= "1" pattern= "container/header/authors/author" /> </digester-rules> so we have two options: rules pattern are wrong; sample is wrong. I am removing the <root> element and see what happens. let you know soon!
        Hide
        Simone Tripodi added a comment -

        Indeed, restructuring the XML to:

        <container>
          <header>
            <authors>
              <author>Author 1</author>
            </authors>
          </header>
        </container>
        

        I am going to commit the test - may I can kindly ask you to checkout the trunk and modify the test there?

        Many thanks in advance!

        Show
        Simone Tripodi added a comment - Indeed, restructuring the XML to: <container> <header> <authors> <author>Author 1</author> </authors> </header> </container> I am going to commit the test - may I can kindly ask you to checkout the trunk and modify the test there? Many thanks in advance!
        Hide
        Torsten Krah added a comment -

        Yeah that was a test for failing parsing, it must be without <root>; however this won't effect the tet case, does still fail in the same way.
        Hm what to modify yet - i am a little bit confused where we are at the moment - yes the test.xml was wrong, but it does not affect the case, right?

        Show
        Torsten Krah added a comment - Yeah that was a test for failing parsing, it must be without <root>; however this won't effect the tet case, does still fail in the same way. Hm what to modify yet - i am a little bit confused where we are at the moment - yes the test.xml was wrong, but it does not affect the case, right?
        Hide
        Simone Tripodi added a comment -

        Unfortunately, once test is modified, test still pass

        Tests run: 209, Failures: 0, Errors: 0, Skipped: 0
        

        this is my environment:

        $ mvn --version
        Apache Maven 3.0.4 (r1232337; 2012-01-17 09:44:56+0100)
        Maven home: /Applications/apache-maven-3.0.4
        Java version: 1.6.0_29, vendor: Apple Inc.
        Java home: /System/Library/Java/JavaVirtualMachines/1.6.0.jdk/Contents/Home
        Default locale: en_US, platform encoding: MacRoman
        OS name: "mac os x", version: "10.7.3", arch: "x86_64", family: "mac"
        

        if you just run:

        https://svn.apache.org/repos/asf/commons/proper/digester/trunk commons-digester && cd commons-digester && svn test
        

        you should see the test report...
        Can you please let me know? TIA!

        Show
        Simone Tripodi added a comment - Unfortunately, once test is modified, test still pass Tests run: 209, Failures: 0, Errors: 0, Skipped: 0 this is my environment: $ mvn --version Apache Maven 3.0.4 (r1232337; 2012-01-17 09:44:56+0100) Maven home: /Applications/apache-maven-3.0.4 Java version: 1.6.0_29, vendor: Apple Inc. Java home: / System /Library/Java/JavaVirtualMachines/1.6.0.jdk/Contents/Home Default locale: en_US, platform encoding: MacRoman OS name: "mac os x" , version: "10.7.3" , arch: "x86_64" , family: "mac" if you just run: https: //svn.apache.org/repos/asf/commons/proper/digester/trunk commons-digester && cd commons-digester && svn test you should see the test report... Can you please let me know? TIA!
        Hide
        Torsten Krah added a comment -

        Patch to testCase

        1. Added test to confirm parsing single threaded.
        2. Modified second one to show parse fail only.

        Show
        Torsten Krah added a comment - Patch to testCase 1. Added test to confirm parsing single threaded. 2. Modified second one to show parse fail only.
        Hide
        Torsten Krah added a comment -

        Output from cmd-line running mvn test - test failed.

        Show
        Torsten Krah added a comment - Output from cmd-line running mvn test - test failed.
        Hide
        Torsten Krah added a comment -

        Patch 2, prevent NPE on failure output on poll == null.

        Show
        Torsten Krah added a comment - Patch 2, prevent NPE on failure output on poll == null.
        Hide
        Simone Tripodi added a comment -

        I'm reviewing it right now; if you intend to continue participating - I hope so! - please, in future patches, don't reformat the original code when providing functional modifications. I suggest you reading the Contributing Patches guideline. Dankeshön

        Show
        Simone Tripodi added a comment - I'm reviewing it right now; if you intend to continue participating - I hope so! - please, in future patches, don't reformat the original code when providing functional modifications. I suggest you reading the Contributing Patches guideline. Dankeshön
        Hide
        Simone Tripodi added a comment -

        wow, this is really wired, after applied the patch I still get a success:

        Tests run: 210, Failures: 0, Errors: 0, Skipped: 0
        
        [INFO] ------------------------------------------------------------------------
        [INFO] BUILD SUCCESS
        [INFO] ------------------------------------------------------------------------
        [INFO] Total time: 1:15.049s
        [INFO] Finished at: Thu Mar 15 20:55:02 CET 2012
        [INFO] Final Memory: 17M/2039M
        [INFO] ------------------------------------------------------------------------
        

        Investigating...

        Show
        Simone Tripodi added a comment - wow, this is really wired, after applied the patch I still get a success: Tests run: 210, Failures: 0, Errors: 0, Skipped: 0 [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS [INFO] ------------------------------------------------------------------------ [INFO] Total time: 1:15.049s [INFO] Finished at: Thu Mar 15 20:55:02 CET 2012 [INFO] Final Memory: 17M/2039M [INFO] ------------------------------------------------------------------------ Investigating...
        Hide
        Simone Tripodi added a comment -

        Ahhhhhh I finally got it... Fix going to be committed in a short while!

        Show
        Simone Tripodi added a comment - Ahhhhhh I finally got it... Fix going to be committed in a short while!
        Hide
        Torsten Krah added a comment -

        Oh sorry for reformatting :-| - my fault, sorry - will take care next time and yes i am still interested in getting this one fixed
        Yeah, i can run it too many times with success. Depends really on the ability to run multiple threads at once to get the threads in the critical path like mentioned where it would fail - may need many times (20+ or 50+) to force some failure here.

        Show
        Torsten Krah added a comment - Oh sorry for reformatting :-| - my fault, sorry - will take care next time and yes i am still interested in getting this one fixed Yeah, i can run it too many times with success. Depends really on the ability to run multiple threads at once to get the threads in the critical path like mentioned where it would fail - may need many times (20+ or 50+) to force some failure here.
        Hide
        Simone Tripodi added a comment -

        I just committed a potential fix for that issue - can I kindly ask you to update your local copy and double check, please?
        TIA!!!

        Show
        Simone Tripodi added a comment - I just committed a potential fix for that issue - can I kindly ask you to update your local copy and double check, please? TIA!!!
        Hide
        Torsten Krah added a comment -

        2 stack traces from test with potential fix - still there for me.

        Show
        Torsten Krah added a comment - 2 stack traces from test with potential fix - still there for me.
        Hide
        Torsten Krah added a comment -

        mvn test from command line run.

        Show
        Torsten Krah added a comment - mvn test from command line run.
        Hide
        Simone Tripodi added a comment -

        OH I finally can see where the issue happens - the problem is in org.apache.commons.digester3.xmlrules.FromXmlRulesModule where there is an attempt to read the same InputSource multiple times ;(

        I need to find an alternative InputSource loading method... otherwise I am forced to discourage the FromXmlRulesModule use in a multiple-thread context...

        Show
        Simone Tripodi added a comment - OH I finally can see where the issue happens - the problem is in org.apache.commons.digester3.xmlrules.FromXmlRulesModule where there is an attempt to read the same InputSource multiple times ;( I need to find an alternative InputSource loading method... otherwise I am forced to discourage the FromXmlRulesModule use in a multiple-thread context...
        Hide
        Torsten Krah added a comment -

        What about initializing the rules at construction time of the loader? Wouldn't it fix the race condition? Or using a ReentrantReadWriteLock to guard the init phase in question?

        Show
        Torsten Krah added a comment - What about initializing the rules at construction time of the loader? Wouldn't it fix the race condition? Or using a ReentrantReadWriteLock to guard the init phase in question?
        Hide
        Simone Tripodi added a comment -

        I just committed r1301217, can you check if it works in your environment please?
        I adopted the strategy of loading the rules stream as soon as the loaders methods are invoked... less performant but safer
        Please let me know, thanks in advance!

        Show
        Simone Tripodi added a comment - I just committed r1301217 , can you check if it works in your environment please? I adopted the strategy of loading the rules stream as soon as the loaders methods are invoked... less performant but safer Please let me know, thanks in advance!
        Hide
        Torsten Krah added a comment -

        No success yet - thats the output now (sometimes it does work, but not everytime).

        Show
        Torsten Krah added a comment - No success yet - thats the output now (sometimes it does work, but not everytime).
        Hide
        Simone Tripodi added a comment -

        HA poor me, I see - the already bound check should be now per-thread, not global, otherwise threads cannot load the same resource concurrently :S
        I'll provide you a new fix attempt ASAP - Thanks for reporting!

        Show
        Simone Tripodi added a comment - HA poor me, I see - the already bound check should be now per-thread, not global, otherwise threads cannot load the same resource concurrently :S I'll provide you a new fix attempt ASAP - Thanks for reporting!
        Hide
        Simone Tripodi added a comment -

        I committed r1301493, I hope this will fix that issue once for all!

        Show
        Simone Tripodi added a comment - I committed r1301493, I hope this will fix that issue once for all!
        Hide
        Torsten Krah added a comment -

        Sorry - seems to be still there.
        Output from 3 of ca. 20 runs attached.
        NullPointerException and ConcurrentModificationExceptions seen.

        Show
        Torsten Krah added a comment - Sorry - seems to be still there. Output from 3 of ca. 20 runs attached. NullPointerException and ConcurrentModificationExceptions seen.
        Hide
        Simone Tripodi added a comment -

        bad, bad, bad news... ;( investigating again...

        Show
        Simone Tripodi added a comment - bad, bad, bad news... ;( investigating again...
        Hide
        Thomas Neidhart added a comment - - edited

        I have no idea about the digester, but I looked at this issue out of curiosity (and I like to debug concurrency problems .

        The problems is in the DigesterLoader#addRules method:

        • createRuleSet clears the underlying rule binder
        • addRuleInstance iterates over the same rule binder

        If you make this method synchronized, you will not get an exception anymore.

        What I do not know, if the createRuleSet is implemented correctly. I would expect that the initialization of the rules binder happens when the DigesterLoader is created, not when a new digester is requested, but this is may be due to lazy loading?

        Show
        Thomas Neidhart added a comment - - edited I have no idea about the digester, but I looked at this issue out of curiosity (and I like to debug concurrency problems . The problems is in the DigesterLoader#addRules method: createRuleSet clears the underlying rule binder addRuleInstance iterates over the same rule binder If you make this method synchronized, you will not get an exception anymore. What I do not know, if the createRuleSet is implemented correctly. I would expect that the initialization of the rules binder happens when the DigesterLoader is created, not when a new digester is requested, but this is may be due to lazy loading?
        Hide
        Simone Tripodi added a comment -

        you are a good observator Thomas

        when I coded createRuleSet the first time, I thought about lazy loading AND to the fact that users can set different ClassLoader}}s - since classes resolution by name can produce different results, I opted for checking it when requesting new {{Digester instances.

        I would like to avoid to add a synchronized block, even if producing Digester instances is quietly fast that would be a bottleneck, thread-safe behavior with concurrent accesses would be preferred.

        Do you have any other suggestion?
        TIA!!!

        Show
        Simone Tripodi added a comment - you are a good observator Thomas when I coded createRuleSet the first time, I thought about lazy loading AND to the fact that users can set different ClassLoader}}s - since classes resolution by name can produce different results, I opted for checking it when requesting new {{Digester instances. I would like to avoid to add a synchronized block, even if producing Digester instances is quietly fast that would be a bottleneck, thread-safe behavior with concurrent accesses would be preferred. Do you have any other suggestion? TIA!!!
        Hide
        Simone Tripodi added a comment -

        Thanks to Thomas' observations, I opted to drop the lazy loading support - please see r1301815

        Can you do yet another test cycle, when having spare time, please? TIA!

        Show
        Simone Tripodi added a comment - Thanks to Thomas' observations, I opted to drop the lazy loading support - please see r1301815 Can you do yet another test cycle, when having spare time, please? TIA!
        Hide
        Torsten Krah added a comment -

        Looks promising until now - tested it a few times yet and seems to be fixed, since initialization is done at construction time now of the loader. I'll reopen it if needed. Thx for getting this fixed.

        Show
        Torsten Krah added a comment - Looks promising until now - tested it a few times yet and seems to be fixed, since initialization is done at construction time now of the loader. I'll reopen it if needed. Thx for getting this fixed.
        Hide
        Simone Tripodi added a comment -

        Thanks Torsten for the feedbacks and thanks Thomas for having a look at it!

        I mark the issue as resolved and Torsten has to feel free to reopen it if needed.

        Show
        Simone Tripodi added a comment - Thanks Torsten for the feedbacks and thanks Thomas for having a look at it! I mark the issue as resolved and Torsten has to feel free to reopen it if needed.

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Torsten Krah
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development