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:
      None

      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.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

          People

          • Assignee:
            Unassigned
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development