Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.4.3, 3.4.4
    • Fix Version/s: 3.4.6, 3.5.0
    • Component/s: c client
    • Labels:
    • Environment:

      Zookeeper client and server both are running on CentOS 6.3

    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      zoo_multi API used to leak memory while deserializing the response from zookeeper server.
      Completion entries for individual operation in zoo_multi transaction weren't getting cleaned causing memory leak. This patch resolves this memory leak by destroying completion entries in deserialize_multi function.
      Show
      zoo_multi API used to leak memory while deserializing the response from zookeeper server. Completion entries for individual operation in zoo_multi transaction weren't getting cleaned causing memory leak. This patch resolves this memory leak by destroying completion entries in deserialize_multi function.
    • Tags:
      zoo_multi memory-leak

      Description

      Valgrind is reporting memory leak for zoo_multi operations.

      ==4056== 2,240 (160 direct, 2,080 indirect) bytes in 1 blocks are definitely lost in loss record 18 of 24
      ==4056== at 0x4A04A28: calloc (vg_replace_malloc.c:467)
      ==4056== by 0x504D822: create_completion_entry (zookeeper.c:2322)
      ==4056== by 0x5052833: zoo_amulti (zookeeper.c:3141)
      ==4056== by 0x5052A8B: zoo_multi (zookeeper.c:3240)

      It looks like completion entries for individual operations in multiupdate transaction are not getting freed. My observation is that memory leak size depends on the number of operations in single mutlipupdate transaction

      1. ZOOKEEPER-1562.patch
        0.6 kB
        Deepak Jagtap

        Activity

        Deepak Jagtap created issue -
        Marshall McMullen made changes -
        Field Original Value New Value
        Assignee Marshall McMullen [ marshall ]
        Hide
        Marshall McMullen added a comment -

        This is certainly a bug in the C multi code I wrote, so i'm taking this bug...

        Show
        Marshall McMullen added a comment - This is certainly a bug in the C multi code I wrote, so i'm taking this bug...
        Hide
        Deepak Jagtap added a comment -

        Hi Marshall,

        Thanks for taking this up!
        Just curious has this memory leak been fixed in 3.4.5 stable release.
        Or it will be included in future releases.
        Also please let know if I can assist you in fixing this.

        Thanks & Regards,
        Deepak

        Show
        Deepak Jagtap added a comment - Hi Marshall, Thanks for taking this up! Just curious has this memory leak been fixed in 3.4.5 stable release. Or it will be included in future releases. Also please let know if I can assist you in fixing this. Thanks & Regards, Deepak
        Hide
        Deepak Jagtap added a comment -

        Hi,

        zoo_multi API used to leak memory while deserializing the response from zookeeper server.
        Completion entries for individual operation in zoo_multi transaction weren't getting cleaned causing memory leak. This patch resolves this memory leak by destroying completion entries in deserialize_multi function.

        Please find attached patch for the same.
        Could you please review it and let me know if you have any comments?

        Thanks & Regards,
        Deepak

        Show
        Deepak Jagtap added a comment - Hi, zoo_multi API used to leak memory while deserializing the response from zookeeper server. Completion entries for individual operation in zoo_multi transaction weren't getting cleaned causing memory leak. This patch resolves this memory leak by destroying completion entries in deserialize_multi function. Please find attached patch for the same. Could you please review it and let me know if you have any comments? Thanks & Regards, Deepak
        Deepak Jagtap made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Release Note zoo_multi API used to leak memory while deserializing the response from zookeeper server.
        Completion entries for individual operation in zoo_multi transaction weren't getting cleaned causing memory leak. This patch resolves this memory leak by destroying completion entries in deserialize_multi function.
        Labels patch
        Fix Version/s 3.4.5 [ 12321883 ]
        Deepak Jagtap made changes -
        Attachment ZOOKEEPER-1562.patch [ 12566893 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12566893/ZOOKEEPER-1562.patch
        against trunk revision 1438375.

        +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/1366//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/12566893/ZOOKEEPER-1562.patch against trunk revision 1438375. +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/1366//console This message is automatically generated.
        Hide
        Marshall McMullen added a comment -

        Deepak, Thanks for the patch. Have been meaning to get to this but way too much on my plate.

        I reviewed the patch and it looks good to me. Pretty simple fix. I assume with this fix all the unit tests still pass and there is no longer a memory leak ?

        If you verify those two things then +1 for me.

        Show
        Marshall McMullen added a comment - Deepak, Thanks for the patch. Have been meaning to get to this but way too much on my plate. I reviewed the patch and it looks good to me. Pretty simple fix. I assume with this fix all the unit tests still pass and there is no longer a memory leak ? If you verify those two things then +1 for me.
        Deepak Jagtap made changes -
        Attachment ZOOKEEPER-1562.patch [ 12566893 ]
        Deepak Jagtap made changes -
        Attachment ZOOKEEPER-1562.patch [ 12567032 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12567032/ZOOKEEPER-1562.patch
        against trunk revision 1438375.

        +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 generated 26 release audit warnings (more than the trunk's current 24 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/1368//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//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/12567032/ZOOKEEPER-1562.patch against trunk revision 1438375. +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 generated 26 release audit warnings (more than the trunk's current 24 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/1368//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1368//console This message is automatically generated.
        Hide
        Deepak Jagtap added a comment -

        Thanks for reviewing this Marshall!
        I have verified that valgrind doesn't report any memory leak with my application.
        But somehow I am not managed to successfully run unit tests provided in the trunk though Hadoop QA reported success for all unit tests. It looks like I am missing some steps in compile and build.

        I am using following steps for compiling, building and running test:
        1. svn checkout https://svn.apache.org/repos/asf/zookeeper/tags/release-3.4.5/ zookeeper-3.4.5
        2. patch -p0 < ZOOKEEPER-1562.patch
        2. cd zookeeper-3.4.5
        3. ant compile_jute
        4. cd zookeeper-3.4.5/src/c
        5. autoreconf -if
        6. ./configure
        7. make
        8. cd ../../../zookeeper-3.4.5
        9. ant test
        On the console I could see many tests failed.
        I tried same thing without my patch and still same test are failing.
        Am I missing any steps in the build and patch testing?

        Thanks & Regards,
        Deepak

        Show
        Deepak Jagtap added a comment - Thanks for reviewing this Marshall! I have verified that valgrind doesn't report any memory leak with my application. But somehow I am not managed to successfully run unit tests provided in the trunk though Hadoop QA reported success for all unit tests. It looks like I am missing some steps in compile and build. I am using following steps for compiling, building and running test: 1. svn checkout https://svn.apache.org/repos/asf/zookeeper/tags/release-3.4.5/ zookeeper-3.4.5 2. patch -p0 < ZOOKEEPER-1562 .patch 2. cd zookeeper-3.4.5 3. ant compile_jute 4. cd zookeeper-3.4.5/src/c 5. autoreconf -if 6. ./configure 7. make 8. cd ../../../zookeeper-3.4.5 9. ant test On the console I could see many tests failed. I tried same thing without my patch and still same test are failing. Am I missing any steps in the build and patch testing? Thanks & Regards, Deepak
        Hide
        Marshall McMullen added a comment -

        Don't do steps 8 and 9. After step 7, do this:

        8. make zktest-st
        9. ./zktest-st
        10. make zktest-mt
        11. ./zktest-mt

        Show
        Marshall McMullen added a comment - Don't do steps 8 and 9. After step 7, do this: 8. make zktest-st 9. ./zktest-st 10. make zktest-mt 11. ./zktest-mt
        Hide
        Deepak Jagtap added a comment -

        Great all tests succeed now.Thanks!

        Show
        Deepak Jagtap added a comment - Great all tests succeed now.Thanks!
        Hide
        Marshall McMullen added a comment -

        Awesome!! +1 from me.

        Show
        Marshall McMullen added a comment - Awesome!! +1 from me.
        Patrick Hunt made changes -
        Assignee Marshall McMullen [ marshall ] Deepak Jagtap [ djagtap ]
        Hide
        Patrick Hunt added a comment -

        Committed to 3.4.6 and trunk. Thanks Deepak! (and Marshall for the review/help)

        Show
        Patrick Hunt added a comment - Committed to 3.4.6 and trunk. Thanks Deepak! (and Marshall for the review/help)
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 3.5.0 [ 12316644 ]
        Fix Version/s 3.4.6 [ 12323310 ]
        Fix Version/s 3.4.5 [ 12321883 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1822 (See https://builds.apache.org/job/ZooKeeper-trunk/1822/)
        ZOOKEEPER-1562. Memory leaks in zoo_multi API (Deepak Jagtap via phunt) (Revision 1441862)

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

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/c/src/zookeeper.c
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1822 (See https://builds.apache.org/job/ZooKeeper-trunk/1822/ ) ZOOKEEPER-1562 . Memory leaks in zoo_multi API (Deepak Jagtap via phunt) (Revision 1441862) Result = SUCCESS phunt : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1441862 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/c/src/zookeeper.c
        Hide
        Flavio Junqueira added a comment -

        Closing issues after releasing 3.4.6.

        Show
        Flavio Junqueira added a comment - Closing issues after releasing 3.4.6.
        Flavio Junqueira made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        108d 1h 52m 1 Deepak Jagtap 29/Jan/13 02:56
        Patch Available Patch Available Resolved Resolved
        5d 3h 45m 1 Patrick Hunt 03/Feb/13 06:42
        Resolved Resolved Closed Closed
        403d 11h 34m 1 Flavio Junqueira 13/Mar/14 18:16

          People

          • Assignee:
            Deepak Jagtap
            Reporter:
            Deepak Jagtap
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development