Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.20.1
    • Fix Version/s: 0.21.0
    • Component/s: libhdfs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      hdfsExists does not call destroyLocalReference for jPath anytime,
      hdfsDelete does not call it when it fails, and
      hdfsRename does not call it for jOldPath and jNewPath when it fails

      1. patch.HADOOP-6034.0.18
        1.0 kB
        Christian Kunz
      2. patch.HADOOP-6034
        3 kB
        Christian Kunz
      3. HADOOP-6034.patch
        4 kB
        Christian Kunz
      4. HDFS-464.patch
        5 kB
        Christian Kunz
      5. hdfs-464.rel20.patch
        4 kB
        Suresh Srinivas
      6. hdfs-464.1.rel20.patch
        4 kB
        Suresh Srinivas

        Issue Links

          Activity

          Christian Kunz created issue -
          Hide
          Christian Kunz added a comment -

          This patch fixes the memory leaks mentioned in the description and some additional ones in

          hdfsGetCapacity
          hdfsGetUsed
          hdfsConnectAsUser
          hdfsConnectAsUserNewInstance

          Show
          Christian Kunz added a comment - This patch fixes the memory leaks mentioned in the description and some additional ones in hdfsGetCapacity hdfsGetUsed hdfsConnectAsUser hdfsConnectAsUserNewInstance
          Christian Kunz made changes -
          Field Original Value New Value
          Attachment patch.HADOOP-6034 [ 12410609 ]
          Christian Kunz made changes -
          Assignee Christian Kunz [ ckunz ]
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410609 ]
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410610 ]
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410610 ]
          Hide
          Christian Kunz added a comment -

          patch.HADOOP-6034 is for trunk
          patch.HADOOP-6034.0.18 is for hadoop-0.18 (just has the memory leaks mentioned in description)

          Show
          Christian Kunz added a comment - patch. HADOOP-6034 is for trunk patch. HADOOP-6034 .0.18 is for hadoop-0.18 (just has the memory leaks mentioned in description)
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410611 ]
          Attachment patch.HADOOP-6034.0.18 [ 12410612 ]
          Christian Kunz made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          dhruba borthakur added a comment -

          Cool catch! +1.

          Show
          dhruba borthakur added a comment - Cool catch! +1.
          Hide
          Hadoop QA added a comment -

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

          +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 tests are needed for this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/512/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/12410612/patch.HADOOP-6034.0.18 against trunk revision 785065. +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 tests are needed for this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/512/console This message is automatically generated.
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410611 ]
          Hide
          Christian Kunz added a comment -

          Reattaching patch for trunk, hopefully allowing hudson to pick up the correct patch

          Show
          Christian Kunz added a comment - Reattaching patch for trunk, hopefully allowing hudson to pick up the correct patch
          Christian Kunz made changes -
          Attachment patch.HADOOP-6034 [ 12410967 ]
          Christian Kunz made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Christian Kunz added a comment -

          Resubmitting patch.

          Show
          Christian Kunz added a comment - Resubmitting patch.
          Christian Kunz made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Tsz Wo Nicholas Sze made changes -
          Link This issue depends on MAPREDUCE-665 [ MAPREDUCE-665 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          libhdfs was accidentally moved to the mapreduce project.. See MAPREDUCE-665.

          Show
          Tsz Wo Nicholas Sze added a comment - libhdfs was accidentally moved to the mapreduce project.. See MAPREDUCE-665 .
          Owen O'Malley made changes -
          Project Hadoop Common [ 12310240 ] Hadoop HDFS [ 12310942 ]
          Key HADOOP-6034 HDFS-464
          Affects Version/s 0.18.3 [ 12313494 ]
          Component/s libhdfs [ 12311345 ]
          Fix Version/s 0.18.4 [ 12313628 ]
          Hide
          Christian Kunz added a comment -

          Updated patch for trunk

          Show
          Christian Kunz added a comment - Updated patch for trunk
          Christian Kunz made changes -
          Attachment HADOOP-6034.patch [ 12426579 ]
          Hide
          Christian Kunz added a comment -

          Now, with libhdfs back in HDFS (finally!) there is no reason not to apply this patch.

          Most of the memory leaks are on the failure path, but there are a couple which happen on the success path as well (hdfsExists, connecting with user id).

          Therefore, I will mark it as blocker.

          BTW, not being a JNI expert, I am not confident that all memory leaks are fixed. There might be more.

          Show
          Christian Kunz added a comment - Now, with libhdfs back in HDFS (finally!) there is no reason not to apply this patch. Most of the memory leaks are on the failure path, but there are a couple which happen on the success path as well (hdfsExists, connecting with user id). Therefore, I will mark it as blocker. BTW, not being a JNI expert, I am not confident that all memory leaks are fixed. There might be more.
          Christian Kunz made changes -
          Affects Version/s 0.20.1 [ 12314048 ]
          Priority Major [ 3 ] Blocker [ 1 ]
          Hide
          Eli Collins added a comment -

          Patch looks good. Did you run the libhdfs test?

          fyi filed HDFS-773 for testing for additional memory leaks (eg run the code under valgrind).

          Show
          Eli Collins added a comment - Patch looks good. Did you run the libhdfs test? fyi filed HDFS-773 for testing for additional memory leaks (eg run the code under valgrind).
          Hide
          Christian Kunz added a comment -

          Thanks for looking at the patch.

          Sorry, I do not have the setup to run libhdfs test.

          We are running the equivalent patch on hadoop-0.20.1 for months now in our applications, and we use libhdfs extensively but, of course, this does not guarantee a comprehensive test.

          Show
          Christian Kunz added a comment - Thanks for looking at the patch. Sorry, I do not have the setup to run libhdfs test. We are running the equivalent patch on hadoop-0.20.1 for months now in our applications, and we use libhdfs extensively but, of course, this does not guarantee a comprehensive test.
          Hide
          Eli Collins added a comment -

          If you've got a build you should be able to run the libhdfs test with:

          ant compile -Dcompile.c++=true -Dlibhdfs=true test-c++-libhdfs
          
          Show
          Eli Collins added a comment - If you've got a build you should be able to run the libhdfs test with: ant compile -Dcompile.c++= true -Dlibhdfs= true test-c++-libhdfs
          Hide
          Christian Kunz added a comment -

          Thanks, that was simple!
          Yes, libhdfs test passed in trunk with patch HADOOP-6034.patch.

          Show
          Christian Kunz added a comment - Thanks, that was simple! Yes, libhdfs test passed in trunk with patch HADOOP-6034 .patch.
          Hide
          Eli Collins added a comment -

          Excellent. +1

          Show
          Eli Collins added a comment - Excellent. +1
          Suresh Srinivas made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Suresh Srinivas made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Suresh Srinivas added a comment -

          Comments:

          1. hdfs.c - In hdfsConnectAsUserNewInstance90 method jAttrString is not released before subsequent returns. Is this fine?
          2. hdfs.c - In hdfsOpenFile(), jpath, jStrBufferSize, jStrReplication, jStrBlockSize is not release before subsequent return (if (!file) check)
          3. hdfs.c - In hdfsGetHosts() does blockHosts leak in subsequent return NULL?

          On a side note, the code seems to be prone to introducing leaks. Seems to me we should have some kind of stack variable that tracks the objects to be deleted and cleans them up when it goes out of scope.

          Show
          Suresh Srinivas added a comment - Comments: hdfs.c - In hdfsConnectAsUserNewInstance90 method jAttrString is not released before subsequent returns. Is this fine? hdfs.c - In hdfsOpenFile(), jpath, jStrBufferSize, jStrReplication, jStrBlockSize is not release before subsequent return (if (!file) check) hdfs.c - In hdfsGetHosts() does blockHosts leak in subsequent return NULL? On a side note, the code seems to be prone to introducing leaks. Seems to me we should have some kind of stack variable that tracks the objects to be deleted and cleans them up when it goes out of scope.
          Hide
          Hadoop QA added a comment -

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

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

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/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/12426579/HADOOP-6034.patch against trunk revision 899747. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/191/console This message is automatically generated.
          Hide
          Christian Kunz added a comment -

          > 1. hdfs.c - In hdfsConnectAsUserNewInstance90 method jAttrString is not released before subsequent returns. Is this fine?
          You are entirely correct that it should be released, similar to jAttrString in hdfsConnectAsUser

          > 2. hdfs.c - In hdfsOpenFile(), jpath, jStrBufferSize, jStrReplication, jStrBlockSize is not release before subsequent return (if (!file) check)
          The original author must have thought that if you cannot malloc a few bytes you are hosed anyhow.

          > 3. hdfs.c - In hdfsGetHosts() does blockHosts leak in subsequent return NULL?
          It is the responsibility of the client to call hdfsFreeHosts, similar to calling hdfsFreeFileInfo after a call to hdfsGetPathInfo.

          > On a side note, the code seems to be prone to introducing leaks. Seems to me we should have some kind of stack variable that tracks the objects to be deleted and cleans them up when it goes out of scope.
          One giant step forward would be to replace the libhfds library (which uses JNI) with a socket interface allowing C++ to directly access hdfs servers (would increase performance and reduce memory footprint)

          Show
          Christian Kunz added a comment - > 1. hdfs.c - In hdfsConnectAsUserNewInstance90 method jAttrString is not released before subsequent returns. Is this fine? You are entirely correct that it should be released, similar to jAttrString in hdfsConnectAsUser > 2. hdfs.c - In hdfsOpenFile(), jpath, jStrBufferSize, jStrReplication, jStrBlockSize is not release before subsequent return (if (!file) check) The original author must have thought that if you cannot malloc a few bytes you are hosed anyhow. > 3. hdfs.c - In hdfsGetHosts() does blockHosts leak in subsequent return NULL? It is the responsibility of the client to call hdfsFreeHosts, similar to calling hdfsFreeFileInfo after a call to hdfsGetPathInfo. > On a side note, the code seems to be prone to introducing leaks. Seems to me we should have some kind of stack variable that tracks the objects to be deleted and cleans them up when it goes out of scope. One giant step forward would be to replace the libhfds library (which uses JNI) with a socket interface allowing C++ to directly access hdfs servers (would increase performance and reduce memory footprint)
          Hide
          Eli Collins added a comment -

          Looks like the patch just needs to be updated to address points 1 and 2 right? By the way it looks like all but four of these cases has been covered by patches Brian Bockelman has posted to various hdfs jiras.

          Show
          Eli Collins added a comment - Looks like the patch just needs to be updated to address points 1 and 2 right? By the way it looks like all but four of these cases has been covered by patches Brian Bockelman has posted to various hdfs jiras.
          Hide
          Suresh Srinivas added a comment -

          If the new patch is uploaded, I will commit this change to trunk, branch 0.21 and 0.20 as well.

          Show
          Suresh Srinivas added a comment - If the new patch is uploaded, I will commit this change to trunk, branch 0.21 and 0.20 as well.
          Hide
          Eli Collins added a comment -

          Don't see the new patch. Christian's response was on Jan 10 and the latest patch is from Dec 1.

          Show
          Eli Collins added a comment - Don't see the new patch. Christian's response was on Jan 10 and the latest patch is from Dec 1.
          Hide
          Christian Kunz added a comment -

          I will upload a new patch shortly (btw, my last comment was on Jan. 16)

          Show
          Christian Kunz added a comment - I will upload a new patch shortly (btw, my last comment was on Jan. 16)
          Hide
          Christian Kunz added a comment -

          New patch fixing some additional memory leaks.

          Show
          Christian Kunz added a comment - New patch fixing some additional memory leaks.
          Christian Kunz made changes -
          Attachment HDFS-464.patch [ 12430814 ]
          Hide
          Christian Kunz added a comment -

          The new patch passed libhdfs test.

          Show
          Christian Kunz added a comment - The new patch passed libhdfs test.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #174 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/174/)
          . Fix memory leaks in libhdfs. Contributed by Christian Kunz.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #174 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/174/ ) . Fix memory leaks in libhdfs. Contributed by Christian Kunz.
          Hide
          Suresh Srinivas added a comment -

          Committed this patch to trunk. Thank you Christian.

          Show
          Suresh Srinivas added a comment - Committed this patch to trunk. Thank you Christian.
          Suresh Srinivas made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.22.0 [ 12314241 ]
          Resolution Fixed [ 1 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Since this is a 0.21 blocker, reopen for committing this to 0.21.

          Show
          Tsz Wo Nicholas Sze added a comment - Since this is a 0.21 blocker, reopen for committing this to 0.21.
          Tsz Wo Nicholas Sze made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this to MAPREDUCE-0.21, which contains libhdfs.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this to MAPREDUCE-0.21, which contains libhdfs.
          Tsz Wo Nicholas Sze made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 0.21.0 [ 12314046 ]
          Resolution Fixed [ 1 ]
          Tsz Wo Nicholas Sze made changes -
          Component/s contrib/libhdfs [ 12313126 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #207 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/207/)
          . Fix memory leaks in libhdfs. Contributed by Christian Kunz.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #207 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/207/ ) . Fix memory leaks in libhdfs. Contributed by Christian Kunz.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #101 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/101/)
          . Fix memory leaks in libhdfs. Contributed by Christian Kunz.

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #101 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/101/ ) . Fix memory leaks in libhdfs. Contributed by Christian Kunz.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #205 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #205 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/205/ )
          Hide
          Suresh Srinivas added a comment -

          Attaching release 20 version of the patch.

          Show
          Suresh Srinivas added a comment - Attaching release 20 version of the patch.
          Suresh Srinivas made changes -
          Attachment hdfs-464.rel20.patch [ 12436647 ]
          Hide
          Christian Kunz added a comment -

          +1 for hdfs-464.rel20.patch except the following section should be deleted (this is when USE_UUGI is not defined, which does not happen)

          @@ -276,12 +281,14 @@ hdfsFS hdfsConnectAsUser(const char* host, tPort port, const char *user , const
                     destroyLocalReference(env, jGroups);
                   }
                   destroyLocalReference(env, jUgi);
          +        destroyLocalReference(env, jAttrString);
                   return NULL;
                 }
                 
                 destroyLocalReference(env, jUserString);
                 destroyLocalReference(env, jGroups);
                 destroyLocalReference(env, jUgi);
          +      destroyLocalReference(env, jAttrString);
               }
           #endif      
               //Check what type of FileSystem the caller wants...
          
          Show
          Christian Kunz added a comment - +1 for hdfs-464.rel20.patch except the following section should be deleted (this is when USE_UUGI is not defined, which does not happen) @@ -276,12 +281,14 @@ hdfsFS hdfsConnectAsUser( const char * host, tPort port, const char *user , const destroyLocalReference(env, jGroups); } destroyLocalReference(env, jUgi); + destroyLocalReference(env, jAttrString); return NULL; } destroyLocalReference(env, jUserString); destroyLocalReference(env, jGroups); destroyLocalReference(env, jUgi); + destroyLocalReference(env, jAttrString); } #endif //Check what type of FileSystem the caller wants...
          Hide
          Suresh Srinivas added a comment -

          Attached new patch file incorporating comments from Christian...

          Show
          Suresh Srinivas added a comment - Attached new patch file incorporating comments from Christian...
          Suresh Srinivas made changes -
          Attachment hdfs-464.1.rel20.patch [ 12436681 ]
          Hide
          Christian Kunz added a comment -

          +1 for hdfs-464.1.rel20.patch
          Thanks for the 0.20 patch, Suresh!

          Show
          Christian Kunz added a comment - +1 for hdfs-464.1.rel20.patch Thanks for the 0.20 patch, Suresh!
          Tom White made changes -
          Fix Version/s 0.22.0 [ 12314241 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Gavin made changes -
          Link This issue depends on MAPREDUCE-665 [ MAPREDUCE-665 ]
          Gavin made changes -
          Link This issue depends upon MAPREDUCE-665 [ MAPREDUCE-665 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Patch Available Patch Available Open Open
          214d 21h 50m 2 Suresh Srinivas 15/Jan/10 22:00
          Open Open Patch Available Patch Available
          1d 18h 13m 3 Suresh Srinivas 15/Jan/10 22:01
          Patch Available Patch Available Resolved Resolved
          5d 5m 1 Suresh Srinivas 20/Jan/10 22:06
          Resolved Resolved Reopened Reopened
          2h 13m 1 Tsz Wo Nicholas Sze 21/Jan/10 00:19
          Reopened Reopened Resolved Resolved
          6m 29s 1 Tsz Wo Nicholas Sze 21/Jan/10 00:25
          Resolved Resolved Closed Closed
          215d 20h 22m 1 Tom White 24/Aug/10 21:48

            People

            • Assignee:
              Christian Kunz
              Reporter:
              Christian Kunz
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development