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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development