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
