Trent Nelson wrote on Mon, Jan 23, 2012 at 15:07:20 -0500: > Fix a memory leak in convert_rangelist by *always* Py_DECREF'ing the object > returned by convert_to_swigtype (a SWIG wrapper around svn_merge_range_t) > once we've established it's not NULL. > > This is required because PyList_Append takes ownership of references passed > in (i.e. calls Py_INCREF). Prior to this fix, the reference counts of any
Nice log message. > svn_merge_range_t objects would always be off-by-one, preventing them from > ever being garbage collected, having dire effects on the memory usage of > long-running programs calling svn_mergeinfo_parse() on real-world data. > > Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c > =================================================================== > --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c > (revision 1234786) > +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c > (working copy) > @@ -653,15 +653,27 @@ > static PyObject *convert_rangelist(void *value, void *ctx, PyObject *py_pool) > { > int i; > + int result; > + PyObject *obj; > PyObject *list; > - apr_array_header_t *array = value; > + apr_array_header_t *array = (apr_array_header_t *)value; > As before: this cast shouldn't need to be explicit. Also, I've moved the two new declarations to a more inner scope. > list = PyList_New(0); > + if (list == NULL) > + return NULL; > + > for (i = 0; i < array->nelts; i++) > { > svn_merge_range_t *range = APR_ARRAY_IDX(array, i, svn_merge_range_t > *); > - if (PyList_Append(list, convert_to_swigtype(range, ctx, py_pool)) == > -1) > + > + obj = convert_to_swigtype(range, (swig_type_info *)ctx, py_pool); > + if (obj == NULL) > goto error; > + > + result = PyList_Append(list, obj); > + Py_DECREF(obj); > + if (result == -1) > + goto error; > } > return list; > error: I then ended up with the following patch: [[[ Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c =================================================================== --- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 1235202) +++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy) @@ -657,10 +657,22 @@ static PyObject *convert_rangelist(void *value, vo apr_array_header_t *array = value; list = PyList_New(0); + if (list == NULL) + return NULL; + for (i = 0; i < array->nelts; i++) { svn_merge_range_t *range = APR_ARRAY_IDX(array, i, svn_merge_range_t *); - if (PyList_Append(list, convert_to_swigtype(range, ctx, py_pool)) == -1) + PyObject *obj; + int result; + + obj = convert_to_swigtype(range, ctx, py_pool); + if (obj == NULL) + goto error; + + result = PyList_Append(list, obj); + Py_DECREF(obj); + if (result == -1); goto error; } return list; ]]] However, with that patch, 'make check-swig-py' fails: [[[ ====================================================================== ERROR: test_mergeinfo_get (mergeinfo.SubversionMergeinfoTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 109, in test_mergeinfo_get False, None, None) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/repos.py", line 650, in svn_repos_fs_get_mergeinfo return _repos.svn_repos_fs_get_mergeinfo(*args) SystemError: error return without exception set ====================================================================== ERROR: Test svn_mergeinfo_merge() ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 75, in test_mergeinfo_merge mergeinfo1 = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse return _core.svn_mergeinfo_parse(*args) SystemError: error return without exception set ====================================================================== ERROR: Test svn_mergeinfo_parse() ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 61, in test_mergeinfo_parse mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse return _core.svn_mergeinfo_parse(*args) SystemError: error return without exception set ====================================================================== ERROR: test_mergeinfo_sort (mergeinfo.SubversionMergeinfoTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 93, in test_mergeinfo_sort mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse return _core.svn_mergeinfo_parse(*args) SystemError: error return without exception set ====================================================================== ERROR: test_rangelist_merge (mergeinfo.SubversionMergeinfoTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 66, in test_rangelist_merge mergeinfo1 = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse return _core.svn_mergeinfo_parse(*args) SystemError: error return without exception set ====================================================================== ERROR: test_rangelist_reverse (mergeinfo.SubversionMergeinfoTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/tests/mergeinfo.py", line 82, in test_rangelist_reverse mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1) File "/home/daniel/src/svn/t1/subversion/bindings/swig/python/libsvn/core.py", line 4550, in svn_mergeinfo_parse return _core.svn_mergeinfo_parse(*args) SystemError: error return without exception set ---------------------------------------------------------------------- ]]] and the failure disappears once I revert the patch. Could you look into that, please? I suspect the C code is buggy, but I'm not sure whether it's due to your patch or to my mangling of it. Thanks, Daniel