Hadoop Common
  1. Hadoop Common
  2. HADOOP-10365

BufferedOutputStream in FileUtil#unpackEntries() should be closed in finally block

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: util
    • Labels:

      Description

          BufferedOutputStream outputStream = new BufferedOutputStream(
              new FileOutputStream(outputFile));
      ...
          outputStream.flush();
          outputStream.close();
      

      outputStream should be closed in finally block.

      1. HADOOP-10365.2.patch
        1 kB
        Sanghyun Yun
      2. HADOOP-10365.3.patch
        1 kB
        Sanghyun Yun
      3. HADOOP-10365.4.patch
        1 kB
        Sanghyun Yun
      4. HADOOP-10365.5.patch
        1 kB
        Kiran Kumar M R
      5. HADOOP-10365.patch
        1 kB
        Sanghyun Yun

        Activity

        Hide
        Sanghyun Yun added a comment -

        I add try-catch-finally block and write outputStream.close() in finally.

        Show
        Sanghyun Yun added a comment - I add try-catch-finally block and write outputStream.close() in finally.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12662721/HADOOP-10365.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl
        org.apache.hadoop.crypto.key.TestValueQueue
        org.apache.hadoop.ipc.TestDecayRpcScheduler
        org.apache.hadoop.ipc.TestCallQueueManager

        The following test timeouts occurred in hadoop-common-project/hadoop-common:

        org.apache.hadoop.http.TestHttpServerLifecycle

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12662721/HADOOP-10365.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl org.apache.hadoop.crypto.key.TestValueQueue org.apache.hadoop.ipc.TestDecayRpcScheduler org.apache.hadoop.ipc.TestCallQueueManager The following test timeouts occurred in hadoop-common-project/hadoop-common: org.apache.hadoop.http.TestHttpServerLifecycle +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4508//console This message is automatically generated.
        Hide
        Tsuyoshi Ozawa added a comment -

        Thanks for your reporting, Sanghyun Yun. As Colin Patrick McCabe told me on HDFS-6902, it's better to use IOUtils.cleanup(LOG, outputStream) in final block. Do you mind updating a patch?

        Show
        Tsuyoshi Ozawa added a comment - Thanks for your reporting, Sanghyun Yun . As Colin Patrick McCabe told me on HDFS-6902 , it's better to use IOUtils.cleanup(LOG, outputStream) in final block. Do you mind updating a patch?
        Hide
        Sanghyun Yun added a comment -

        Tsuyoshi Ozawa, Thanks for your review! I modified code using IOUtils.cleanup() method.

        Show
        Sanghyun Yun added a comment - Tsuyoshi Ozawa , Thanks for your review! I modified code using IOUtils.cleanup() method.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.ha.TestZKFailoverControllerStress

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverControllerStress +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4548//console This message is automatically generated.
        Hide
        Sanghyun Yun added a comment -

        Please review my patch, Tsuyoshi Ozawa and Ted Yu.

        Show
        Sanghyun Yun added a comment - Please review my patch, Tsuyoshi Ozawa and Ted Yu .
        Hide
        Tsuyoshi Ozawa added a comment -

        Sanghyun Yun, thanks for updating a patch. I rethought of the usage of IOUtils - in fact, unpackEntries is only called from FileUtil#unTarUsingJava. FileUtil#unTarUsingJava closes all inputStream in finally block if there are any exceptions in unpackEntries. So, I think we don't need to cleanup the descriptors in unpackEntries. What do you think?

        Show
        Tsuyoshi Ozawa added a comment - Sanghyun Yun , thanks for updating a patch. I rethought of the usage of IOUtils - in fact, unpackEntries is only called from FileUtil#unTarUsingJava. FileUtil#unTarUsingJava closes all inputStream in finally block if there are any exceptions in unpackEntries. So, I think we don't need to cleanup the descriptors in unpackEntries. What do you think?
        Hide
        Sanghyun Yun added a comment -

        Thanks for your review, Tsuyoshi Ozawa.
        I think tis or inputStream in FileUtil#unTarUsingJava is differrent from outputStream in FileUtil#unpackEntries.
        outputStream is created newly in FileUtil#unpackEntries. So, need to cleanup.

        Show
        Sanghyun Yun added a comment - Thanks for your review, Tsuyoshi Ozawa . I think tis or inputStream in FileUtil#unTarUsingJava is differrent from outputStream in FileUtil#unpackEntries. outputStream is created newly in FileUtil#unpackEntries. So, need to cleanup.
        Hide
        Akira AJISAKA added a comment -

        So, need to cleanup.

        Agree. The patch looks mostly good. Minor nits:
        1.

        +    } catch (IOException e) {
        +      throw e;
        

        These lines are no-op, so they can be removed.
        2.

        +      outputStream = new BufferedOutputStream(new FileOutputStream(outputFile))
        

        Would you please render the line within 80 characters?

        Show
        Akira AJISAKA added a comment - So, need to cleanup. Agree. The patch looks mostly good. Minor nits: 1. + } catch (IOException e) { + throw e; These lines are no-op, so they can be removed. 2. + outputStream = new BufferedOutputStream( new FileOutputStream(outputFile)) Would you please render the line within 80 characters?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch
        against trunk revision 46f6f9d.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

        org.apache.hadoop.ha.TestZKFailoverControllerStress

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12664355/HADOOP-10365.2.patch against trunk revision 46f6f9d. +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ha.TestZKFailoverControllerStress +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5065//console This message is automatically generated.
        Hide
        Tsuyoshi Ozawa added a comment -

        Sanghyun Yun Sorry for the delay. Rethinking of using IOUtils - in this case, I found that we should raise IOException to unTarUsingJava and stop the execution immediately when FileOutputStream#close raises IOException. So, could you update the patch based on v1 including comments by Akira?

        Show
        Tsuyoshi Ozawa added a comment - Sanghyun Yun Sorry for the delay. Rethinking of using IOUtils - in this case, I found that we should raise IOException to unTarUsingJava and stop the execution immediately when FileOutputStream#close raises IOException. So, could you update the patch based on v1 including comments by Akira?
        Hide
        Sanghyun Yun added a comment -

        Thanks for comments, Akira AJISAKA and Tsuyoshi Ozawa.
        I agree that FileUtil#unpackEntries() raise the exception.
        So, I attached a new patch file.

        Show
        Sanghyun Yun added a comment - Thanks for comments, Akira AJISAKA and Tsuyoshi Ozawa . I agree that FileUtil#unpackEntries() raise the exception. So, I attached a new patch file.
        Hide
        Tsuyoshi Ozawa added a comment -

        Sanghyun Yun thanks for your quick updating. Minor nits: could you fix following lines to conform to coding rule?
        http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449

        if (outputStream != null) outputStream.close();
        
        if (outputStream != null) {
          outputStream.close();
        }
        
        Show
        Tsuyoshi Ozawa added a comment - Sanghyun Yun thanks for your quick updating. Minor nits: could you fix following lines to conform to coding rule? http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449 if (outputStream != null ) outputStream.close(); if (outputStream != null ) { outputStream.close(); }
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12681025/HADOOP-10365.3.patch
        against trunk revision 53f64ee.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5067//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5067//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12681025/HADOOP-10365.3.patch against trunk revision 53f64ee. +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5067//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5067//console This message is automatically generated.
        Hide
        Sanghyun Yun added a comment -

        Sorry that breaking the coding rules.
        I modified and reattached.
        Thanks, Tsuyoshi Ozawa.

        Show
        Sanghyun Yun added a comment - Sorry that breaking the coding rules. I modified and reattached. Thanks, Tsuyoshi Ozawa .
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12681185/HADOOP-10365.4.patch
        against trunk revision d7150a1.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5071//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5071//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12681185/HADOOP-10365.4.patch against trunk revision d7150a1. +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5071//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5071//console This message is automatically generated.
        Hide
        Tsuyoshi Ozawa added a comment -

        +1, LGTM.

        Show
        Tsuyoshi Ozawa added a comment - +1, LGTM.
        Hide
        Tsuyoshi Ozawa added a comment -

        Sanghyun Yun Sorry for iterative comments - how about using try-with-resources since we only support JDK7+ now. Do you mind updating it?
        http://docs.oracle.com/javase/7/docs/technotes/guides/language/try-with-resources.html

        Show
        Tsuyoshi Ozawa added a comment - Sanghyun Yun Sorry for iterative comments - how about using try-with-resources since we only support JDK7+ now. Do you mind updating it? http://docs.oracle.com/javase/7/docs/technotes/guides/language/try-with-resources.html
        Hide
        Kiran Kumar M R added a comment -

        Taking this issue as part of Bug Bash. Sanghyun Yun if you are still interested in working on this feel free to assign to yourself.
        I have attached a patch using try-with-resources. Tsuyoshi Ozawa review the patch.

        Show
        Kiran Kumar M R added a comment - Taking this issue as part of Bug Bash. Sanghyun Yun if you are still interested in working on this feel free to assign to yourself. I have attached a patch using try-with-resources. Tsuyoshi Ozawa review the patch.
        Hide
        Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 15m 6s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 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 javac 7m 40s There were no new javac warning messages.
        +1 javadoc 9m 49s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 1m 7s The applied patch generated 217 new checkstyle issues (total was 107, now 215).
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 39s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 42s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
        +1 common tests 23m 11s Tests passed in hadoop-common.
            61m 15s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12731558/HADOOP-10365.5.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / d0e75e6
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/diffcheckstylehadoop-common.txt
        hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/testrun_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/testReport/
        Java 1.7.0_55
        uname Linux asf903.gq1.ygridcore.net 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
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/console

        This message was automatically generated.

        Show
        Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 15m 6s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 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 javac 7m 40s There were no new javac warning messages. +1 javadoc 9m 49s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 7s The applied patch generated 217 new checkstyle issues (total was 107, now 215). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 39s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 42s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 23m 11s Tests passed in hadoop-common.     61m 15s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12731558/HADOOP-10365.5.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / d0e75e6 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/diffcheckstylehadoop-common.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/testReport/ Java 1.7.0_55 uname Linux asf903.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/6555/console This message was automatically generated.

          People

          • Assignee:
            Kiran Kumar M R
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:

              Development