Details

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

      Description

      Both the Listener and Responder thread call Selector.open but don't have a matching .close(). This causes a leak of anonymous pipes. Not a big deal because people rarely close and re-open servers, but worth fixing.

      1. hadoop-7146.txt
        3 kB
        Todd Lipcon
      2. hadoop-7146.txt
        6 kB
        Todd Lipcon
      3. hadoop-7146.txt
        7 kB
        Todd Lipcon
      4. fd-leak.txt
        0.9 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          here's a fix, but should add a unit test. There's a way you can use platform mxbeans to find the number of open FDs, but I can't quite track down the right API at the moment.

          Show
          Todd Lipcon added a comment - here's a fix, but should add a unit test. There's a way you can use platform mxbeans to find the number of open FDs, but I can't quite track down the right API at the moment.
          Hide
          Todd Lipcon added a comment -

          Couldn't find a way to do the test with MXBeans. This patch just uses /proc to see how many FDs are open, and uses junit's Assume support so it won't run on systems where that's not available.

          Show
          Todd Lipcon added a comment - Couldn't find a way to do the test with MXBeans. This patch just uses /proc to see how many FDs are open, and uses junit's Assume support so it won't run on systems where that's not available.
          Hide
          Aaron T. Myers added a comment -

          +1, patch looks great, Todd.

          I also ran TestDFSStorageStateRecovery with this patch applied in the course of debugging HDFS-1884, and can confirm that this patch fixes that test as well.

          Show
          Aaron T. Myers added a comment - +1, patch looks great, Todd. I also ran TestDFSStorageStateRecovery with this patch applied in the course of debugging HDFS-1884 , and can confirm that this patch fixes that test as well.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479955/hadoop-7146.txt
          against trunk revision 1125221.

          +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 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//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/12479955/hadoop-7146.txt against trunk revision 1125221. +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 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/501//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          Todd, kudos for finding this and how to fix it, and also to Aaron for nailing the TestDFSStorageStateRecovery problem.

          I'd like to suggest a more structured fix:
          1. Move the "readSelector = Selector.open()" statement from Listener() to the Reader() ctor, and change the ctor to have no arguments.
          2. In Reader.run(), wrap the whole method in a try/finally context, with your five-line fix in the "finally" clause, and make it conditional on (readSelector != null).
          3. In Responder.run(), similarly wrap the whole method in a try/finally context, with your five-line fix in the "finally" clause, and make it conditional on (writeSelector != null).

          I would have liked to recommend putting the "readSelector = Selector.open()" [Reader] and "writeSelector = Selector.open()" [Responder] statements at the beginning of their respective run() methods, so the try/finally context would really be structured right, but I'm not sure its okay in the Responder context – it looks like some of its methods may invoke writeSelector without the run() being running.

          Show
          Matt Foley added a comment - Todd, kudos for finding this and how to fix it, and also to Aaron for nailing the TestDFSStorageStateRecovery problem. I'd like to suggest a more structured fix: 1. Move the "readSelector = Selector.open()" statement from Listener() to the Reader() ctor, and change the ctor to have no arguments. 2. In Reader.run(), wrap the whole method in a try/finally context, with your five-line fix in the "finally" clause, and make it conditional on (readSelector != null). 3. In Responder.run(), similarly wrap the whole method in a try/finally context, with your five-line fix in the "finally" clause, and make it conditional on (writeSelector != null). I would have liked to recommend putting the "readSelector = Selector.open()" [Reader] and "writeSelector = Selector.open()" [Responder] statements at the beginning of their respective run() methods, so the try/finally context would really be structured right, but I'm not sure its okay in the Responder context – it looks like some of its methods may invoke writeSelector without the run() being running.
          Hide
          Todd Lipcon added a comment -

          I took all of your suggestions, except I didn't add null checks - the variables in question are never reassigned to null, so there's no way they could be, as far as I can tell.

          Show
          Todd Lipcon added a comment - I took all of your suggestions, except I didn't add null checks - the variables in question are never reassigned to null, so there's no way they could be, as far as I can tell.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12480303/hadoop-7146.txt
          against trunk revision 1127215.

          +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 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//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/12480303/hadoop-7146.txt against trunk revision 1127215. +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 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/518//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          I didn't add null checks - the variables in question are never reassigned to null, so there's no way they could be, as far as I can tell.

          Quite right. To safeguard that, could you please make those two variables "final"?

          Minor: In Reader.doRunLoop(), the LOG.Info() could use ", e" instead of "+ StringUtils.stringifyException(e)"

          +1 if those small changes are okay with you.

          Show
          Matt Foley added a comment - I didn't add null checks - the variables in question are never reassigned to null, so there's no way they could be, as far as I can tell. Quite right. To safeguard that, could you please make those two variables "final"? Minor: In Reader.doRunLoop(), the LOG.Info() could use ", e" instead of "+ StringUtils.stringifyException(e)" +1 if those small changes are okay with you.
          Hide
          Todd Lipcon added a comment -

          New patch addressing Matt's latest comments.

          I did change the log messages, though they weren't new from this patch. I agree that it's better not to use stringify, though.

          Show
          Todd Lipcon added a comment - New patch addressing Matt's latest comments. I did change the log messages, though they weren't new from this patch. I agree that it's better not to use stringify, though.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12480424/hadoop-7146.txt
          against trunk revision 1127215.

          +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 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//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/12480424/hadoop-7146.txt against trunk revision 1127215. +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 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/525//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          +1. Looks great! Thanks, Todd.

          Show
          Matt Foley added a comment - +1. Looks great! Thanks, Todd.
          Hide
          Todd Lipcon added a comment -

          Committed to 22 and trunk. Thanks Matt and Aaron

          Show
          Todd Lipcon added a comment - Committed to 22 and trunk. Thanks Matt and Aaron
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #57 (See https://builds.apache.org/hudson/job/Hadoop-Common-22-branch/57/)
          HADOOP-7146. RPC server leaks file descriptors. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.22/src/java/org/apache/hadoop/ipc/Server.java
          • /hadoop/common/branches/branch-0.22/src/test/core/org/apache/hadoop/ipc/TestIPC.java
          • /hadoop/common/branches/branch-0.22/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #57 (See https://builds.apache.org/hudson/job/Hadoop-Common-22-branch/57/ ) HADOOP-7146 . RPC server leaks file descriptors. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127815 Files : /hadoop/common/branches/branch-0.22/src/java/org/apache/hadoop/ipc/Server.java /hadoop/common/branches/branch-0.22/src/test/core/org/apache/hadoop/ipc/TestIPC.java /hadoop/common/branches/branch-0.22/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #623 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/623/)
          HADOOP-7146. RPC server leaks file descriptors. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/ipc/Server.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/ipc/TestIPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #623 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/623/ ) HADOOP-7146 . RPC server leaks file descriptors. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127811 Files : /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/ipc/Server.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/ipc/TestIPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/)
          HADOOP-7146. RPC server leaks file descriptors. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/ipc/Server.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/ipc/TestIPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/ ) HADOOP-7146 . RPC server leaks file descriptors. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127811 Files : /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/ipc/Server.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/ipc/TestIPC.java

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development