Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-1618

static structMap in FieldMetaData is not thread safe and can lead to deadlocks

Details

    • Bug
    • Status: Closed
    • Critical
    • Resolution: Fixed
    • 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8
    • 0.9.3
    • None
    • jdk 1.6

    Description

      Generated thrift structs use static initializers to add metadata to the static 'structMap' field in FieldMetaData. Since the thrift structs may be separate classes, the static initializers can be executed on different threads. The addStructMetaDataMap method does not synchronize access and uses a standard HashMap which is not thread safe. This can lead to corruption of the structMap and in some cases deadlocks due to corrupt entries in the map during a put operation.

      The easy fix is to use a thread safe map for the structMap.

      Another fix is to just retrieve the field directly from the class in the 'getStructMetaDataMap' using reflection.

      I've confirmed the deadlock in 0.2. I looked at the 0.8 code and confirmed the bug is still present.

      Attachments

        1. ThriftDeadlock.java
          4 kB
          Mike Rettig

        Activity

          mrettig Mike Rettig added a comment -

          Here is my repro. You will need to provide your own jar that includes some generated thrift classes.

          mrettig Mike Rettig added a comment - Here is my repro. You will need to provide your own jar that includes some generated thrift classes.
          roger Roger Meier added a comment -

          do you have a patch or/and a test case?

          roger Roger Meier added a comment - do you have a patch or/and a test case?
          mrettig Mike Rettig added a comment -

          ThriftDeadlock.java was attached on september 27th. It just needs a jar that contains some thrift classes and it should deadlock just about every time it runs.

          mrettig Mike Rettig added a comment - ThriftDeadlock.java was attached on september 27th. It just needs a jar that contains some thrift classes and it should deadlock just about every time it runs.
          mrettig Mike Rettig added a comment -

          Still appears broken.

          https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob_plain;f=lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java

          The static map is not thread safe, but it might be accessed from multiple threads which can corrupt the map.

          mrettig Mike Rettig added a comment - Still appears broken. https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob_plain;f=lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java The static map is not thread safe, but it might be accessed from multiple threads which can corrupt the map.
          githubbot ASF GitHub Bot added a comment -

          GitHub user dhelder opened a pull request:

          https://github.com/apache/thrift/pull/235

          THRIFT-1618: synchronize access to hashtable in FieldMetaData

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/dhelder/thrift THRIFT-1618

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/235.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #235


          commit b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09
          Author: David Helder <david@twitter.com>
          Date: 2014-10-03T16:50:14Z

          THRIFT-1618: synchronize access to hashtable in FieldMetaData


          githubbot ASF GitHub Bot added a comment - GitHub user dhelder opened a pull request: https://github.com/apache/thrift/pull/235 THRIFT-1618 : synchronize access to hashtable in FieldMetaData You can merge this pull request into a Git repository by running: $ git pull https://github.com/dhelder/thrift THRIFT-1618 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/235.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #235 commit b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09 Author: David Helder <david@twitter.com> Date: 2014-10-03T16:50:14Z THRIFT-1618 : synchronize access to hashtable in FieldMetaData
          hudson Hudson added a comment -

          FAILURE: Integrated in Thrift-precommit #11 (See https://builds.apache.org/job/Thrift-precommit/11/)
          THRIFT-1618: synchronize access to hashtable in FieldMetaData (david: rev b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09)

          • lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java
          • lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java
          hudson Hudson added a comment - FAILURE: Integrated in Thrift-precommit #11 (See https://builds.apache.org/job/Thrift-precommit/11/ ) THRIFT-1618 : synchronize access to hashtable in FieldMetaData (david: rev b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09) lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java
          hudson Hudson added a comment -

          FAILURE: Integrated in Thrift-precommit #30 (See https://builds.apache.org/job/Thrift-precommit/30/)
          THRIFT-1618: synchronize access to hashtable in FieldMetaData (david: rev b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09)

          • lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java
          • lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java
          hudson Hudson added a comment - FAILURE: Integrated in Thrift-precommit #30 (See https://builds.apache.org/job/Thrift-precommit/30/ ) THRIFT-1618 : synchronize access to hashtable in FieldMetaData (david: rev b7a0158fbaf9c2e2b82e021711ad12eacfdcfc09) lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java
          githubbot ASF GitHub Bot added a comment -

          GitHub user dhelder opened a pull request:

          https://github.com/apache/thrift/pull/348

          THRIFT-1618: synchronize access to hashtable in FieldMetaData

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/dhelder/thrift THRIFT-1618

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/348.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #348


          commit 14cfe39d296a8502d7e192053906a4a631f5cdd2
          Author: David Helder <david@twitter.com>
          Date: 2015-01-06T19:59:41Z

          THRIFT-1618: synchronize access to hashtable in FieldMetaData


          githubbot ASF GitHub Bot added a comment - GitHub user dhelder opened a pull request: https://github.com/apache/thrift/pull/348 THRIFT-1618 : synchronize access to hashtable in FieldMetaData You can merge this pull request into a Git repository by running: $ git pull https://github.com/dhelder/thrift THRIFT-1618 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/348.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #348 commit 14cfe39d296a8502d7e192053906a4a631f5cdd2 Author: David Helder <david@twitter.com> Date: 2015-01-06T19:59:41Z THRIFT-1618 : synchronize access to hashtable in FieldMetaData
          githubbot ASF GitHub Bot added a comment -

          Github user dhelder closed the pull request at:

          https://github.com/apache/thrift/pull/235

          githubbot ASF GitHub Bot added a comment - Github user dhelder closed the pull request at: https://github.com/apache/thrift/pull/235
          githubbot ASF GitHub Bot added a comment - Github user dhelder commented on the pull request: https://github.com/apache/thrift/pull/235#issuecomment-68924155 Replaced with https://github.com/apache/thrift/pull/348
          roger Roger Meier added a comment -

          Thanks David!

          roger Roger Meier added a comment - Thanks David!
          hudson Hudson added a comment -

          SUCCESS: Integrated in Thrift #1427 (See https://builds.apache.org/job/Thrift/1427/)
          THRIFT-1618: synchronize access to hashtable in FieldMetaData (roger: rev 4a78c6eb8670cbb664a199b1c98518033e51e525)

          • lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java
          • lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java
          hudson Hudson added a comment - SUCCESS: Integrated in Thrift #1427 (See https://builds.apache.org/job/Thrift/1427/ ) THRIFT-1618 : synchronize access to hashtable in FieldMetaData (roger: rev 4a78c6eb8670cbb664a199b1c98518033e51e525) lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java
          githubbot ASF GitHub Bot added a comment -

          Github user bufferoverflow commented on the pull request:

          https://github.com/apache/thrift/pull/348#issuecomment-69010748

          Thanks David!
          committed 4a78c6eb8670cbb664a199b1c98518033e51e525
          could you please close this PR
          -roger

          githubbot ASF GitHub Bot added a comment - Github user bufferoverflow commented on the pull request: https://github.com/apache/thrift/pull/348#issuecomment-69010748 Thanks David! committed 4a78c6eb8670cbb664a199b1c98518033e51e525 could you please close this PR -roger
          githubbot ASF GitHub Bot added a comment -

          Github user dhelder commented on the pull request:

          https://github.com/apache/thrift/pull/348#issuecomment-69068535

          Merged. Thanks!

          githubbot ASF GitHub Bot added a comment - Github user dhelder commented on the pull request: https://github.com/apache/thrift/pull/348#issuecomment-69068535 Merged. Thanks!
          githubbot ASF GitHub Bot added a comment -

          Github user dhelder closed the pull request at:

          https://github.com/apache/thrift/pull/348

          githubbot ASF GitHub Bot added a comment - Github user dhelder closed the pull request at: https://github.com/apache/thrift/pull/348

          People

            Unassigned Unassigned
            mrettig Mike Rettig
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: