Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13179

GenericOptionsParser is not thread-safe because commons-cli OptionBuilder is not thread-safe

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      I'm running into similar issues like http://stackoverflow.com/questions/22462665/is-hadoops-toorunner-thread-safe, the author's observation seem to make sense to me. However when I checked the hadoop github trunk I found the issue still not fixed.

      Chris Nauroth further investigated this issue, here's his quote:

      The root cause is that commons-cli OptionBuilder is not thread-safe.

      https://commons.apache.org/proper/commons-cli/apidocs/org/apache/commons/cl
      i/OptionBuilder.html

      According to this issue, commons-cli doesn't plan to change that and
      instead chose to document the lack of thread-safety.

      https://issues.apache.org/jira/browse/CLI-209

      I think we can solve this in Hadoop, probably with a one-line change to
      make GenericOptionsParser#buildGeneralOptions a synchronized method.

      I'll soon upload a patch for this

      1. HADOOP-13179-master.patch
        2 kB
        hongbin ma
      2. HADOOP-13179.001.patch
        1 kB
        hongbin ma

        Issue Links

          Activity

          Hide
          cnauroth Chris Nauroth added a comment -

          Steve Loughran, I just realized my last comment was pretty terse and might have looked dismissive. If so, I apologize. It was good for us to get this small patch in, because it definitely addresses the one specific case of the bug that was reported. However, I agree with you about fixing throughout the codebase. I linked known instances of the bug to your HADOOP-13391 issue. Thank you for filing that.

          Show
          cnauroth Chris Nauroth added a comment - Steve Loughran , I just realized my last comment was pretty terse and might have looked dismissive. If so, I apologize. It was good for us to get this small patch in, because it definitely addresses the one specific case of the bug that was reported. However, I agree with you about fixing throughout the codebase. I linked known instances of the bug to your HADOOP-13391 issue. Thank you for filing that.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          OK. In YARN-679 I go synchronized(OptionsBuilder.class); that is what should be done throughout the codebase. I'll file a JIRA

          Show
          stevel@apache.org Steve Loughran added a comment - OK. In YARN-679 I go synchronized(OptionsBuilder.class) ; that is what should be done throughout the codebase. I'll file a JIRA
          Hide
          cnauroth Chris Nauroth added a comment -

          Looking at the code, I don't think this actually fixes the problem of concurrent access to OptionsBuilder...there are lots of uses of the class in the Hadoop codebase, and they are all synchronized off different things.

          That's correct. The scope of this patch was limited to thread safety of GenericOptionsParser.

          Show
          cnauroth Chris Nauroth added a comment - Looking at the code, I don't think this actually fixes the problem of concurrent access to OptionsBuilder ...there are lots of uses of the class in the Hadoop codebase, and they are all synchronized off different things. That's correct. The scope of this patch was limited to thread safety of GenericOptionsParser .
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Looking at the code, I don't think this actually fixes the problem of concurrent access to OptionsBuilder...there are lots of uses of the class in the Hadoop codebase, and they are all synchronized off different things.

          The way to do this safely would be to make them all synchronized(OptionsBuilder)

          Show
          stevel@apache.org Steve Loughran added a comment - Looking at the code, I don't think this actually fixes the problem of concurrent access to OptionsBuilder ...there are lots of uses of the class in the Hadoop codebase, and they are all synchronized off different things. The way to do this safely would be to make them all synchronized(OptionsBuilder)
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This just broke my never-gets-reviewed YARN-679 patch, where I'd allowed for services to be able to define their own options parser. Not your fault, just one of those details which arises when patches don't get reviewed in a timely manner. I'l have to fix my code there.

          FWIW, I think we should jump to JCommander for future work: introspection+annotation based, much easier to work with.

          Show
          stevel@apache.org Steve Loughran added a comment - This just broke my never-gets-reviewed YARN-679 patch, where I'd allowed for services to be able to define their own options parser. Not your fault, just one of those details which arises when patches don't get reviewed in a timely manner. I'l have to fix my code there. FWIW, I think we should jump to JCommander for future work: introspection+annotation based, much easier to work with.
          Hide
          vinayrpet Vinayakumar B added a comment -

          Committed to trunk, branch-2 and branch-2.8.

          Thanks hongbin ma for the contribution.
          Thanks Chris Nauroth for the suggestions and review.

          Show
          vinayrpet Vinayakumar B added a comment - Committed to trunk, branch-2 and branch-2.8. Thanks hongbin ma for the contribution. Thanks Chris Nauroth for the suggestions and review.
          Hide
          mahongbin hongbin ma added a comment -

          thanks

          any other thing I can do to merge to code into trunk?

          Show
          mahongbin hongbin ma added a comment - thanks any other thing I can do to merge to code into trunk?
          Hide
          vinayrpet Vinayakumar B added a comment - - edited

          I thought you can even make buildGeneralOptions() non-static and remove the SupressWarnings annotation.

          Oops. Next second I realized, you cannot make it non-static and synchronized since we want to block multiple access to static OptionsBuilder. So buildGeneralOptions() has to be static, otherwise whole purpose of this jira will not be served.

          Current fix works. +1.

          Show
          vinayrpet Vinayakumar B added a comment - - edited I thought you can even make buildGeneralOptions() non-static and remove the SupressWarnings annotation. Oops. Next second I realized, you cannot make it non-static and synchronized since we want to block multiple access to static OptionsBuilder . So buildGeneralOptions() has to be static, otherwise whole purpose of this jira will not be served. Current fix works. +1.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 7m 36s trunk passed
          +1 compile 8m 25s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 1m 15s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 27s trunk passed
          +1 javadoc 1m 3s trunk passed
          +1 mvninstall 0m 43s the patch passed
          +1 compile 7m 59s the patch passed
          +1 javac 7m 59s the patch passed
          +1 checkstyle 0m 23s the patch passed
          +1 mvnsite 0m 53s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 55s the patch passed
          +1 unit 8m 7s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          42m 25s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805192/HADOOP-13179.001.patch
          JIRA Issue HADOOP-13179
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 038526fd2e17 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0287c49
          Default Java 1.8.0_91
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9535/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9535/console
          Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 36s trunk passed +1 compile 8m 25s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 1m 15s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 27s trunk passed +1 javadoc 1m 3s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 7m 59s the patch passed +1 javac 7m 59s the patch passed +1 checkstyle 0m 23s the patch passed +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 55s the patch passed +1 unit 8m 7s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 42m 25s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805192/HADOOP-13179.001.patch JIRA Issue HADOOP-13179 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 038526fd2e17 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0287c49 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9535/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9535/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          mahongbin hongbin ma added a comment -

          thanks Chris Nauroth, I merely copied the code changes in http://stackoverflow.com/questions/22462665/is-hadoops-toorunner-thread-safe without too much think.

          Just uploaded the modified version with correct name. thank you!

          Show
          mahongbin hongbin ma added a comment - thanks Chris Nauroth , I merely copied the code changes in http://stackoverflow.com/questions/22462665/is-hadoops-toorunner-thread-safe without too much think. Just uploaded the modified version with correct name. thank you!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 docker 13m 57s Docker failed to build yetus/hadoop:1886bab.



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12804850/HADOOP-13179-master.patch
          JIRA Issue HADOOP-13179
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9531/console
          Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 docker 13m 57s Docker failed to build yetus/hadoop:1886bab. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12804850/HADOOP-13179-master.patch JIRA Issue HADOOP-13179 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9531/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          cnauroth Chris Nauroth added a comment -

          Hello hongbin ma. Thank you for the patch.

          I was thinking that it would be better to make GenericOptionsParser#buildGeneralOptions a synchronized method. All of the usage of the non-thread-safe OptionBuilder class is inside that method. By making that method synchronized, we'd fix the problem for all potential users of the GenericOptionsParser class, not just usage through ToolRunner.

          Could you please take a look at our wiki page on how to contribute?

          https://wiki.apache.org/hadoop/HowToContribute

          Specifically, there are some instructions in there about how to name patch file attachments so that they trigger our pre-commit testing.

          Show
          cnauroth Chris Nauroth added a comment - Hello hongbin ma . Thank you for the patch. I was thinking that it would be better to make GenericOptionsParser#buildGeneralOptions a synchronized method. All of the usage of the non-thread-safe OptionBuilder class is inside that method. By making that method synchronized , we'd fix the problem for all potential users of the GenericOptionsParser class, not just usage through ToolRunner . Could you please take a look at our wiki page on how to contribute? https://wiki.apache.org/hadoop/HowToContribute Specifically, there are some instructions in there about how to name patch file attachments so that they trigger our pre-commit testing.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 docker 0m 3s Docker failed to build yetus/hadoop:1886bab.



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12804850/HADOOP-13179-master.patch
          JIRA Issue HADOOP-13179
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9505/console
          Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 docker 0m 3s Docker failed to build yetus/hadoop:1886bab. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12804850/HADOOP-13179-master.patch JIRA Issue HADOOP-13179 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9505/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

            People

            • Assignee:
              mahongbin hongbin ma
              Reporter:
              mahongbin hongbin ma
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development