Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.3
    • Fix Version/s: 3.3.6, 3.4.4, 3.5.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Environment:

      RHEL 6.0, self-built from 3.3.3 sources

      Description

      I'm seeing a memory leakage when using the "aget" method.

      It leaks tuples and dicts, both containing "stats".

      1. pyzk-mem-leak-fix.diff
        2 kB
        Kapil Thangavelu
      2. zk.patch
        2 kB
        André Cruz
      3. zktest3.py
        0.6 kB
        johan rydberg
      4. zktest4.py
        0.5 kB
        johan rydberg

        Activity

        Hide
        jrydberg johan rydberg added a comment -

        Here's two test cases reproducing the error.

        Both use pykeeper (ease_install pykeeper) to set things up, but then use the zkpython bindings to do the actual testing.

        zktest3.py leaks, zktest4.py does not.

        Show
        jrydberg johan rydberg added a comment - Here's two test cases reproducing the error. Both use pykeeper (ease_install pykeeper) to set things up, but then use the zkpython bindings to do the actual testing. zktest3.py leaks, zktest4.py does not.
        Hide
        jrydberg johan rydberg added a comment -

        https://github.com/apache/zookeeper/blob/trunk/src/contrib/zkpython/src/c/zookeeper.c#L487

        Shouldn't arglist be decref'ed after the call?

        That drops my tuple count from ~55k to ~5k after a zktest3.py run.

        Show
        jrydberg johan rydberg added a comment - https://github.com/apache/zookeeper/blob/trunk/src/contrib/zkpython/src/c/zookeeper.c#L487 Shouldn't arglist be decref'ed after the call? That drops my tuple count from ~55k to ~5k after a zktest3.py run.
        Hide
        kapilt Kapil Thangavelu added a comment -

        This bug applies to ALL async api calls with python.

        Show
        kapilt Kapil Thangavelu added a comment - This bug applies to ALL async api calls with python.
        Hide
        kapilt Kapil Thangavelu added a comment -

        Fix for async python api memory leaks.

        Show
        kapilt Kapil Thangavelu added a comment - Fix for async python api memory leaks.
        Hide
        kapilt Kapil Thangavelu added a comment -

        Noting the problem is generic, it also applies to all extant versions 3.3x and 3.4

        Show
        kapilt Kapil Thangavelu added a comment - Noting the problem is generic, it also applies to all extant versions 3.3x and 3.4
        Hide
        kapilt Kapil Thangavelu added a comment -

        Also wanted to note fwiw, we're going to apply the patch i attached to this issue, in the zookeeper 3.3.5 pkg distributed in ubuntu 12.04/precise.

        Show
        kapilt Kapil Thangavelu added a comment - Also wanted to note fwiw, we're going to apply the patch i attached to this issue, in the zookeeper 3.3.5 pkg distributed in ubuntu 12.04/precise.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12519666/pyzk-mem-leak-fix.diff
        against trunk revision 1330043.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1050//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12519666/pyzk-mem-leak-fix.diff against trunk revision 1330043. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1050//console This message is automatically generated.
        Hide
        phunt Patrick Hunt added a comment -

        Kapil the patch is failing to apply. Please update it and re-attach, thanks. (you might check if the patch applies cleanly to both 3.4 and trunk branches, it might be that it only applies to one, in which case you would need to submit two patches, one for each branch). thanks!

        Show
        phunt Patrick Hunt added a comment - Kapil the patch is failing to apply. Please update it and re-attach, thanks. (you might check if the patch applies cleanly to both 3.4 and trunk branches, it might be that it only applies to one, in which case you would need to submit two patches, one for each branch). thanks!
        Hide
        edevil André Cruz added a comment -

        Corrected patch that applies to trunk and 3.4 branch.

        Show
        edevil André Cruz added a comment - Corrected patch that applies to trunk and 3.4 branch.
        Hide
        hadoopqa Hadoop QA added a comment -

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

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12526345/zk.patch against trunk revision 1336467. +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. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1070//console This message is automatically generated.
        Hide
        pagiannaros Paul Giannaros added a comment -

        Is this patch going to be applied soon? Are you waiting for justification on why it doesn't include new tests?

        Show
        pagiannaros Paul Giannaros added a comment - Is this patch going to be applied soon? Are you waiting for justification on why it doesn't include new tests?
        Hide
        edevil André Cruz added a comment -

        I would also like to know. Tests were not included because it's not easy to test for a memory leak.

        Show
        edevil André Cruz added a comment - I would also like to know. Tests were not included because it's not easy to test for a memory leak.
        Hide
        henryr Henry Robinson added a comment -

        Patch looks good, I'll commit shortly.

        Show
        henryr Henry Robinson added a comment - Patch looks good, I'll commit shortly.
        Hide
        henryr Henry Robinson added a comment -

        Just committed to 3.3, 3.4 and 3.5. Thanks Kapil and Andre!

        Show
        henryr Henry Robinson added a comment - Just committed to 3.3, 3.4 and 3.5. Thanks Kapil and Andre!
        Hide
        hudson Hudson added a comment -

        Integrated in ZooKeeper-trunk #1590 (See https://builds.apache.org/job/ZooKeeper-trunk/1590/)
        ZOOKEEPER-1431. zkpython async calls leak memory (Kapil Thangavelu and Andre Cruz via henryr) (Revision 1351541)

        Result = SUCCESS
        henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1351541
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c
        Show
        hudson Hudson added a comment - Integrated in ZooKeeper-trunk #1590 (See https://builds.apache.org/job/ZooKeeper-trunk/1590/ ) ZOOKEEPER-1431 . zkpython async calls leak memory (Kapil Thangavelu and Andre Cruz via henryr) (Revision 1351541) Result = SUCCESS henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1351541 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c

          People

          • Assignee:
            kapilt Kapil Thangavelu
            Reporter:
            jrydberg johan rydberg
          • Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development