Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.3
    • Fix Version/s: 1.7.4
    • Component/s: java
    • Labels:
      None
    • Environment:

      OSX, JDK6

      Description

      I experience a dead lock when running multiple DataFileReader in concurrent threads.
      See test case here:
      https://github.com/slandelle/avro-test

      AvroBinaryEncodingTest randomly stalls about 50% of the time.
      A thread dump would show a dead lock in org.apache.avro.io.parsing.Symbol class initialization.

      IHMO, the problem is that the Symbol class has static final members that are instances of Symbol subclasses.

      I built a custom version of avro where all the constants (NULL, BOOLEAN, INT, etc) have been extracted into a dedicated class outside of Symbol hierarchy and the test case now runs fine.

      Cheers,

      Stéphane Landelle

      1. AVRO-1220.txt
        9 kB
        Doug Cutting
      2. AVRO-1220.txt
        1 kB
        Doug Cutting

        Activity

        Stéphane Landelle created issue -
        Hide
        Doug Cutting added a comment -

        Moving these public constants to a separate class would be an incompatible API change, so that would be an unfortunate solution.

        I wonder if instead we might instead explicitly initialize all of the Symbol subclasses? Here's a patch that attempts this. Does this help? It's pretty ugly code, I admit.

        Show
        Doug Cutting added a comment - Moving these public constants to a separate class would be an incompatible API change, so that would be an unfortunate solution. I wonder if instead we might instead explicitly initialize all of the Symbol subclasses? Here's a patch that attempts this. Does this help? It's pretty ugly code, I admit.
        Doug Cutting made changes -
        Field Original Value New Value
        Attachment AVRO-1220.txt [ 12560866 ]
        Hide
        Philip Zeyliger added a comment -

        Doug--

        Could we just initialize in a static block once and for all at class loading?

        – Philip

        Show
        Philip Zeyliger added a comment - Doug-- Could we just initialize in a static block once and for all at class loading? – Philip
        Hide
        Doug Cutting added a comment -

        The problem is that, when different subclasses of Symbol are loaded in different threads, each then loads Symbol itself, which then loads those subclasses while binding constants, and things deadlock.

        I've attached a better fix that removes all public constructors for Symbol subclasses and replaces them with factory methods on Symbol. So none of the subclasses are loaded until after Symbol itself is loaded and all of its constants are bound.

        I tried to test this using the code linked in the description of this issue. Without the patch things deadlock. With the patch they fail with:

        JSON record should::be readable when a nullable field has been added to the schema(com.axa.ags.avro.AvroJsonEncodingTest): Expected field name not found: foo

        Is that expected?

        I tried to build a simple, standalone Java test case for this but was unable to get things to deadlock.

        If this new patch looks reasonable then I'll probably re-add the public constructors and deprecate them for back-compatibility. I removed them just to be certain that they were no longer called anywhere.

        Show
        Doug Cutting added a comment - The problem is that, when different subclasses of Symbol are loaded in different threads, each then loads Symbol itself, which then loads those subclasses while binding constants, and things deadlock. I've attached a better fix that removes all public constructors for Symbol subclasses and replaces them with factory methods on Symbol. So none of the subclasses are loaded until after Symbol itself is loaded and all of its constants are bound. I tried to test this using the code linked in the description of this issue. Without the patch things deadlock. With the patch they fail with: JSON record should::be readable when a nullable field has been added to the schema(com.axa.ags.avro.AvroJsonEncodingTest): Expected field name not found: foo Is that expected? I tried to build a simple, standalone Java test case for this but was unable to get things to deadlock. If this new patch looks reasonable then I'll probably re-add the public constructors and deprecate them for back-compatibility. I removed them just to be certain that they were no longer called anywhere.
        Doug Cutting made changes -
        Attachment AVRO-1220.txt [ 12561020 ]
        Hide
        Doug Cutting added a comment -

        Stéphane, can you please confirm whether this patch fixes things for you? Thanks!

        Show
        Doug Cutting added a comment - Stéphane, can you please confirm whether this patch fixes things for you? Thanks!
        Doug Cutting made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Fix Version/s 1.7.4 [ 12323742 ]
        Hide
        Doug Cutting added a comment -

        Stéphane, have you had a chance to look at this?

        Show
        Doug Cutting added a comment - Stéphane, have you had a chance to look at this?
        Hide
        Stéphane Landelle added a comment -

        Hi,

        No, not yet. I'll be off for 2 weeks, I'll have a look then.

        Cheers,

        Stéphane

        2013/1/18 Doug Cutting (JIRA) <jira@apache.org>

        Show
        Stéphane Landelle added a comment - Hi, No, not yet. I'll be off for 2 weeks, I'll have a look then. Cheers, Stéphane 2013/1/18 Doug Cutting (JIRA) <jira@apache.org>
        Hide
        Doug Cutting added a comment -

        Stéphane, if you (or someone else) can confirm this fix then we can include it in Avro 1.7.4.

        Show
        Doug Cutting added a comment - Stéphane, if you (or someone else) can confirm this fix then we can include it in Avro 1.7.4.
        Hide
        Stéphane Landelle added a comment -

        Hi,

        I just tried your second patch and I confirm that the deadlock is gone.

        Thanks and sorry for the late reply.

        Cheers,

        Stéphane

        Show
        Stéphane Landelle added a comment - Hi, I just tried your second patch and I confirm that the deadlock is gone. Thanks and sorry for the late reply. Cheers, Stéphane
        Hide
        Doug Cutting added a comment -

        I committed this.

        Show
        Doug Cutting added a comment - I committed this.
        Doug Cutting made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Assignee Doug Cutting [ cutting ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in AvroJava #346 (See https://builds.apache.org/job/AvroJava/346/)
        AVRO-1220. Java: Fix a deadlock when reading by replacing parser symbol constructors with factory methods. (Revision 1444297)

        Result = SUCCESS
        cutting :
        Files :

        • /avro/trunk/CHANGES.txt
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/JsonGrammarGenerator.java
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/Symbol.java
        • /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ValidatingGrammarGenerator.java
        Show
        Hudson added a comment - Integrated in AvroJava #346 (See https://builds.apache.org/job/AvroJava/346/ ) AVRO-1220 . Java: Fix a deadlock when reading by replacing parser symbol constructors with factory methods. (Revision 1444297) Result = SUCCESS cutting : Files : /avro/trunk/CHANGES.txt /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/JsonGrammarGenerator.java /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ResolvingGrammarGenerator.java /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/Symbol.java /avro/trunk/lang/java/avro/src/main/java/org/apache/avro/io/parsing/ValidatingGrammarGenerator.java
        Doug Cutting made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Doug Cutting
            Reporter:
            Stéphane Landelle
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development