ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-600

TODO pondering about allocation behavior in zkpython may be removed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.2.1
    • Fix Version/s: 3.3.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I suppose the TODO below is referring to the "path" variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns.

      Index: src/contrib/zkpython/src/c/zookeeper.c
      ===================================================================
      — src/contrib/zkpython/src/c/zookeeper.c (revision 885582)
      +++ src/contrib/zkpython/src/c/zookeeper.c (working copy)
      @@ -774,8 +774,6 @@

      static PyObject *pyzoo_get_children(PyObject *self, PyObject *args)
      {

      • // TO DO: Does Python copy the string or the reference? If it's the former
      • // we should free the String_vector
        int zkhid;
        char *path;
        PyObject *watcherfn = Py_None;

        Activity

        Gustavo Niemeyer created issue -
        Patrick Hunt made changes -
        Field Original Value New Value
        Assignee Gustavo Niemeyer [ niemeyer ]
        Patrick Hunt made changes -
        Issue Type Task [ 3 ] Bug [ 1 ]
        Fix Version/s 3.3.0 [ 12313976 ]
        Affects Version/s 3.2.1 [ 12314068 ]
        Component/s contrib-bindings [ 12312860 ]
        Hide
        Patrick Hunt added a comment -

        Hi Gustavo, thanks for the submit. I need to point out that we require submissions via patch file, details of which you can find here:
        http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute
        (use svn diff to create a patch, attach it to this jira using "attach file" link on the left hand side of this page)

        The reason for this is that we need to capture your acceptance of the license grant to ASF. Otw we cannot
        accept the patch (for legal reasons). Also our automated workflow checks submissions and such, it's triggered
        by your attaching the file, then clicking on "submit patch". Thanks for your patience.

        If you could attach you change as a patch file that would be great.

        Show
        Patrick Hunt added a comment - Hi Gustavo, thanks for the submit. I need to point out that we require submissions via patch file, details of which you can find here: http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute (use svn diff to create a patch, attach it to this jira using "attach file" link on the left hand side of this page) The reason for this is that we need to capture your acceptance of the license grant to ASF. Otw we cannot accept the patch (for legal reasons). Also our automated workflow checks submissions and such, it's triggered by your attaching the file, then clicking on "submit patch". Thanks for your patience. If you could attach you change as a patch file that would be great.
        Hide
        Steven Cheng added a comment -

        Hi Gustavo, I think it's talking about the struct String_vector strings that gets copied at the end of the function.

        Show
        Steven Cheng added a comment - Hi Gustavo, I think it's talking about the struct String_vector strings that gets copied at the end of the function.
        Hide
        Henry Robinson added a comment -

        Correct - my concern is over whether PyString_FromString copies the C string it is given. If it does not, then a String_vector is allocated by zoo_wgetchildren and never freed.

        http://docs.python.org/c-api/string.html hints that a copy is made, and therefore the memory needs freeing.

        This would be relatively easy to check by memsetting all strings in the String_vector to 'A' after the copy, and then checking to see if the Python-side strings are altered. Alternatively, you could check the Python C API source - it should be fairly clear what the answer is.

        Thanks for picking this up!

        Show
        Henry Robinson added a comment - Correct - my concern is over whether PyString_FromString copies the C string it is given. If it does not, then a String_vector is allocated by zoo_wgetchildren and never freed. http://docs.python.org/c-api/string.html hints that a copy is made, and therefore the memory needs freeing. This would be relatively easy to check by memsetting all strings in the String_vector to 'A' after the copy, and then checking to see if the Python-side strings are altered. Alternatively, you could check the Python C API source - it should be fairly clear what the answer is. Thanks for picking this up!
        Hide
        Gustavo Niemeyer added a comment -

        Ah, I see.

        Yes, PyString_FromString will definitely copy the string over, so the strings.data array is leaking if it has data allocated dynamically.

        In addition to this, PyString_FromString and PyList_New are both allocating memory, and thus they may fail to return a proper object. This isn't being checked currently.

        Show
        Gustavo Niemeyer added a comment - Ah, I see. Yes, PyString_FromString will definitely copy the string over, so the strings.data array is leaking if it has data allocated dynamically. In addition to this, PyString_FromString and PyList_New are both allocating memory, and thus they may fail to return a proper object. This isn't being checked currently.
        Hide
        Gustavo Niemeyer added a comment -

        Patrick, thanks for pointing me to the conventions, and sorry for missing it earlier.

        I'll give it a shot and submit a patch soon.

        Show
        Gustavo Niemeyer added a comment - Patrick, thanks for pointing me to the conventions, and sorry for missing it earlier. I'll give it a shot and submit a patch soon.
        Hide
        Patrick Hunt added a comment -

        no worries, we don't expect first time contributors to know everything. thanks for the interest.

        Show
        Patrick Hunt added a comment - no worries, we don't expect first time contributors to know everything. thanks for the interest.
        Hide
        Gustavo Niemeyer added a comment -

        The attached patch should fix this problem. It also reuses existing code, and fixes a few other issues around the former problem, with return values not being properly checked for errors.

        I'm afraid there are several instances of variables which are not checked for error conditions in the module, unfortunately. I'm not going to try fixing these in this JIRA, though.

        Show
        Gustavo Niemeyer added a comment - The attached patch should fix this problem. It also reuses existing code, and fixes a few other issues around the former problem, with return values not being properly checked for errors. I'm afraid there are several instances of variables which are not checked for error conditions in the module, unfortunately. I'm not going to try fixing these in this JIRA, though.
        Gustavo Niemeyer made changes -
        Attachment deallocate-vector.patch [ 12426565 ]
        Hide
        Henry Robinson added a comment -

        Patch applies fine for me and tests all pass - looks good, thanks Gustavo! I'll open a JIRA for the other issues.

        Show
        Henry Robinson added a comment - Patch applies fine for me and tests all pass - looks good, thanks Gustavo! I'll open a JIRA for the other issues.
        Hide
        Henry Robinson added a comment -

        Marking as patch submitted so that hudsonqabot can take a look.

        Show
        Henry Robinson added a comment - Marking as patch submitted so that hudsonqabot can take a look.
        Henry Robinson made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12426565/deallocate-vector.patch
        against trunk revision 888216.

        +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 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 passed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/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/12426565/deallocate-vector.patch against trunk revision 888216. +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 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        henry/gustavo,
        is this ready to commit?

        Show
        Mahadev konar added a comment - henry/gustavo, is this ready to commit?
        Hide
        Gustavo Niemeyer added a comment -

        I believe it's ready for integration.

        Show
        Gustavo Niemeyer added a comment - I believe it's ready for integration.
        Hide
        Henry Robinson added a comment -

        Yes, this looks good to me - I'd like to see a test included, but we have no infrastructure for C-side tests which this would probably need. Can be committed as far as I am concerned. Thanks Gustavo!

        Show
        Henry Robinson added a comment - Yes, this looks good to me - I'd like to see a test included, but we have no infrastructure for C-side tests which this would probably need. Can be committed as far as I am concerned. Thanks Gustavo!
        Hide
        Mahadev konar added a comment -

        great ... I just committed this. thanks gustavo.

        Show
        Mahadev konar added a comment - great ... I just committed this. thanks gustavo.
        Mahadev konar made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #634 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/634/)
        . TODO pondering about allocation behavior in zkpython may be removed (gustavo via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #634 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/634/ ) . TODO pondering about allocation behavior in zkpython may be removed (gustavo via mahadev)
        Patrick Hunt 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
        10d 20h 29m 1 Henry Robinson 11/Dec/09 18:52
        Patch Available Patch Available Resolved Resolved
        6d 4h 46m 1 Mahadev konar 17/Dec/09 23:39
        Resolved Resolved Closed Closed
        98d 17h 46m 1 Patrick Hunt 26/Mar/10 17:25

          People

          • Assignee:
            Gustavo Niemeyer
            Reporter:
            Gustavo Niemeyer
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development