Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2132

Potential resource leak in EditLogFileOutputStream.close

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      EditLogFileOutputStream.close(...) sequentially closes a series of underlying resources. If any of the calls to close() throw before the last one, the later resources will never be closed.

      1. hdfs-2132.3.patch
        5 kB
        Aaron T. Myers
      2. hdfs-2132.2.patch
        5 kB
        Aaron T. Myers
      3. hdfs-2132.1.patch
        5 kB
        Aaron T. Myers
      4. hdfs-2132.0.patch
        2 kB
        Aaron T. Myers

        Activity

        Hide
        Aaron T. Myers added a comment -

        Patch which makes sure that EditLogFileOutputStream.close(...) cleans up after itself.

        Show
        Aaron T. Myers added a comment - Patch which makes sure that EditLogFileOutputStream.close(...) cleans up after itself.
        Hide
        Ravi Prakash added a comment -

        I am new to Hadoop so please forgive me if I do not understand the philosophies behind this patch. If any of the close methods fail, they will throw an IOException which will be propagated up the stack. Isn't this the way all JAVA works?
        Comments on your patch
        1. In normal operation all close methods within the try will be called once, and then once again in the IOUtils.cleanup method. What purpose does this serve? I would rather the methods be called only once.
        2. In the finally block, all IOExceptions which might have been thrown are logged, and then programmatically swallowed. The upstream functions are never made aware of these IOExceptions and I am not sure this is the right behavior.

        Show
        Ravi Prakash added a comment - I am new to Hadoop so please forgive me if I do not understand the philosophies behind this patch. If any of the close methods fail, they will throw an IOException which will be propagated up the stack. Isn't this the way all JAVA works? Comments on your patch 1. In normal operation all close methods within the try will be called once, and then once again in the IOUtils.cleanup method. What purpose does this serve? I would rather the methods be called only once. 2. In the finally block, all IOExceptions which might have been thrown are logged, and then programmatically swallowed. The upstream functions are never made aware of these IOExceptions and I am not sure this is the right behavior.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485477/hdfs-2132.0.patch
        against trunk revision 1143147.

        +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 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

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

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

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/880//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/880//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/880//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/12485477/hdfs-2132.0.patch against trunk revision 1143147. +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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/880//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/880//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/880//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Whoops, uploaded the wrong patch. Here's one with tests.

        Show
        Aaron T. Myers added a comment - Whoops, uploaded the wrong patch. Here's one with tests.
        Hide
        Aaron T. Myers added a comment -

        I am new to Hadoop so please forgive me if I do not understand the philosophies behind this patch. If any of the close methods fail, they will throw an IOException which will be propagated up the stack. Isn't this the way all JAVA works?

        This is indeed the way it works and is the desired behavior. The point of this patch is that when a close fails for any one of the Closeables, we should still make a last-ditch effort to close the others. If we can't close them then, there's nothing we can do.

        1. In normal operation all close methods within the try will be called once, and then once again in the IOUtils.cleanup method. What purpose does this serve? I would rather the methods be called only once.

        In the normal case all of the Closeables will be set to null. Note that IOUtils.cleanup(...) expressly handles nulls, and will not attempt to call close() again.

        2. In the finally block, all IOExceptions which might have been thrown are logged, and then programmatically swallowed. The upstream functions are never made aware of these IOExceptions and I am not sure this is the right behavior.

        It's true that in the exceptional case any failures to call close() in IOUtils.cleanup(...) will be logged and not propagated. This is exactly the intended behavior. Note that the original exception caused by the call to close() outside of IOUtils.cleanup(...) will still be propagated up.

        Show
        Aaron T. Myers added a comment - I am new to Hadoop so please forgive me if I do not understand the philosophies behind this patch. If any of the close methods fail, they will throw an IOException which will be propagated up the stack. Isn't this the way all JAVA works? This is indeed the way it works and is the desired behavior. The point of this patch is that when a close fails for any one of the Closeables , we should still make a last-ditch effort to close the others. If we can't close them then, there's nothing we can do. 1. In normal operation all close methods within the try will be called once, and then once again in the IOUtils.cleanup method. What purpose does this serve? I would rather the methods be called only once. In the normal case all of the Closeables will be set to null . Note that IOUtils.cleanup(...) expressly handles nulls , and will not attempt to call close() again. 2. In the finally block, all IOExceptions which might have been thrown are logged, and then programmatically swallowed. The upstream functions are never made aware of these IOExceptions and I am not sure this is the right behavior. It's true that in the exceptional case any failures to call close() in IOUtils.cleanup(...) will be logged and not propagated. This is exactly the intended behavior. Note that the original exception caused by the call to close() outside of IOUtils.cleanup(...) will still be propagated up.
        Hide
        Todd Lipcon added a comment -

        hrm, it doesn't look like bufReady should ever throw an IOE on close, right? it's just a memory buffer.

        But, fc.truncate() might throw an IOE - that seems like the more realistic case to worry about. Maybe that would be a better fault to inject for the test?

        Show
        Todd Lipcon added a comment - hrm, it doesn't look like bufReady should ever throw an IOE on close, right? it's just a memory buffer. But, fc.truncate() might throw an IOE - that seems like the more realistic case to worry about. Maybe that would be a better fault to inject for the test?
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, Todd. Here's a patch which addresses your comment.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, Todd. Here's a patch which addresses your comment.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485480/hdfs-2132.1.patch
        against trunk revision 1143147.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

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

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

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/881//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/881//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/881//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/12485480/hdfs-2132.1.patch against trunk revision 1143147. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/881//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/881//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/881//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Thanks Aaron for the explanation! I agree.

        I might be missing a trick (again ) , but are you sure the Closeables will be null after .close()? Won't they be references pointing to a closed stream) and so close() will be called twice on them. I don't see an easy way to avoid that though. So cool.

        Show
        Ravi Prakash added a comment - Thanks Aaron for the explanation! I agree. I might be missing a trick (again ) , but are you sure the Closeables will be null after .close()? Won't they be references pointing to a closed stream) and so close() will be called twice on them. I don't see an easy way to avoid that though. So cool.
        Hide
        Aaron T. Myers added a comment -

        Thanks Aaron for the explanation!

        No problem.

        I might be missing a trick (again ) , but are you sure the Closeables will be null after .close()?

        Well, now that I look at it, you've effectively caught a bug.

        The previous code was expressly setting bufReady and bufCurrent to null, but not fp or fc. My patch didn't touch that code, but it might as well fix it. I'll upload another patch in a moment.

        Show
        Aaron T. Myers added a comment - Thanks Aaron for the explanation! No problem. I might be missing a trick (again ) , but are you sure the Closeables will be null after .close()? Well, now that I look at it, you've effectively caught a bug. The previous code was expressly setting bufReady and bufCurrent to null , but not fp or fc . My patch didn't touch that code, but it might as well fix it. I'll upload another patch in a moment.
        Hide
        Aaron T. Myers added a comment -

        Patch which addresses the issue Ravi pointed out.

        Show
        Aaron T. Myers added a comment - Patch which addresses the issue Ravi pointed out.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485485/hdfs-2132.2.patch
        against trunk revision 1143147.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

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

        -1 core tests. The patch failed these core unit tests:

        -1 contrib tests. The patch failed contrib unit tests.

        -1 system test framework. The patch failed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/882//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/882//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/882//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/12485485/hdfs-2132.2.patch against trunk revision 1143147. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: -1 contrib tests. The patch failed contrib unit tests. -1 system test framework. The patch failed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/882//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/882//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/882//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485496/hdfs-2132.3.patch
        against trunk revision 1143147.

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

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

        -1 core tests. The patch failed these core unit tests:
        org.apache.hadoop.hdfs.TestFileAppend2

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/887//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/887//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/887//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/12485496/hdfs-2132.3.patch against trunk revision 1143147. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestFileAppend2 +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/887//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/887//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/887//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        I'm confident the failing test is unrelated to this change.

        Show
        Aaron T. Myers added a comment - I'm confident the failing test is unrelated to this change.
        Hide
        Todd Lipcon added a comment -

        seems good to me. +1

        for future reference, you can use junit's assertNull() instead of assertEquals with null, but not worth making a new patch.

        Show
        Todd Lipcon added a comment - seems good to me. +1 for future reference, you can use junit's assertNull() instead of assertEquals with null, but not worth making a new patch.
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the reviews, Todd and Ravi. I've just committed this.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the reviews, Todd and Ravi. I've just committed this.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #781 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/781/)
        HDFS-2132. Potential resource leak in EditLogFileOutputStream.close. (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145428
        Files :

        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #781 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/781/ ) HDFS-2132 . Potential resource leak in EditLogFileOutputStream.close. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145428 Files : /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #722 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/722/)
        HDFS-2132. Potential resource leak in EditLogFileOutputStream.close. (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145428
        Files :

        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #722 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/722/ ) HDFS-2132 . Potential resource leak in EditLogFileOutputStream.close. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145428 Files : /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileOutputStream.java

          People

          • Assignee:
            Aaron T. Myers
            Reporter:
            Aaron T. Myers
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development