On 2020/05/22 8:02, Yasuhito FUTATSUKI wrote:
> On 2020/05/20 21:54, Jun Omae wrote:
>> On Mon, May 18, 2020 at 8:25 AM Yasuhito FUTATSUKI <futat...@poem.co.jp>
>> wrote:
>>> Jun and I found it is caused by long existing bug on SWIG Python bindings,
>>> since before swig-py3 branch was merged (thus this bug exists on 1.13.x
>>> and 1.10.3 branches). On SWIG Python bindings, APIs which have multiple
>>> apr_pool_t * argments cannot be wrapped correctly.
>>>
>>> If we call such a wrapper function with specifying multiple pool argments
>>> explicitly, only last one is used for *all* apr_pool_t * arguments when
>>> it calls C API.
>>
>> Created global-py-pool-ref-count.diff patch to solve the multiple apr_pool *
>> pointers issue in Python bindings.
>>
>> Verified no assertions while running check-swig-py with the following
>> environments:
>>
>> - (Python 3.5.2, 3.6.10, 3.7.7, 3.8.3, 3.9.0a6) x (SWIG 4.0.1)
>> - (Python 2.7.12) x (SWIG 1.3.40, 2.0.12, 3.0.12)
>
> Thank you very much!
>
> With this patch, I confirmed that each apr_pool * argment in C API supplied
> from corresponding pool object if it is supplied by the Python wrapper
> function
> (by reading _wrap_svn_client_propget5() in svn_client.c generated by
> SWIG 3.0.12, for Python 2).
>
> However, I couldn't confirm that the "_parent_pool" attribute in wrapper
> object returned by wrapper function point to "result_pool" object in such
> case,
> yet. I doubt it still points to "scratch_pool" object because variable
> "_global_py_pool" points to "scratch_pool" object when both of "result_pool"
> and "scratch_pool" are provided on such APIs.
In "out" and "argout" typemaps, "_parent_pool" attribute of wrapper objects
are set from "_global_py_pool".
With this additional hunks for SubversionClientTestCase.test_conflict(),
[[[
Index: subversion/bindings/swig/python/tests/client.py
===================================================================
--- subversion/bindings/swig/python/tests/client.py (revision 1877963)
+++ subversion/bindings/swig/python/tests/client.py (working copy)
@@ -592,10 +603,14 @@
client.update4((path,), rev, core.svn_depth_unknown, True, False, False,
False, False, self.client_ctx)
- pool = core.Pool()
- conflict = client.conflict_get(trunk_path, self.client_ctx, pool)
+ result_pool = core.Pool()
+ scratch_pool = core.Pool()
+ conflict = client.conflict_get(trunk_path, self.client_ctx,
+ result_pool, scratch_pool)
self.assertTrue(isinstance(conflict, client.svn_client_conflict_t))
+ self.assertNotEqual(conflict._parent_pool, scratch_pool)
+ self.assertEqual(conflict._parent_pool, result_pool)
conflict_opts = client.conflict_tree_get_resolution_options(conflict,
self.client_ctx)
@@ -602,7 +617,8 @@
self.assertTrue(isinstance(conflict_opts, list))
self.assert_all_instances_of(conflict_opts,
client.svn_client_conflict_option_t)
- pool.clear()
+ scratch_pool.clear()
+ result_pool.clear()
@unittest.skip("experimental API, not currently exposed")
def test_shelf(self):
]]]
this test fails.
[[[
======================================================================
FAIL: test_conflict (client.SubversionClientTestCase)
Test conflict api.
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/home/futatuki/work/subversion/vwc/trunk/subversion/bindings/swig/python/tests/client.py",
line 612, in test_conflict
self.assertNotEqual(conflict._parent_pool, scratch_pool)
AssertionError: <libsvn.core.apr_pool_t; proxy of <Swig Object of type
'apr_pool_t *' at 0x80acf5180> > == <libsvn.core.apr_pool_t; proxy of <Swig
Object of type 'apr_pool_t *' at 0x80acf5180> >
----------------------------------------------------------------------
]]]
I think it can be fixed by overriding _global_py_pool to $input in "in"
typemap, when $1 is updated. It assumes that there are no APIs that
uses two or more result_pool like apr_pool_t * argument for allocationg
return value.
I tweaked Jun's patch to do it.
(Attached file: global-py-pool-ref-count2-diff.txt)
Tested on FreeBSD 11, with Python 2.7(debug), Python 3.7(non debug)
Cheers,
--
Yasuhito FUTATSUKI <futat...@poem.co.jp>/<futat...@yf.bsdclub.org>
Handle correctly multiple apr_pool_t * arguments of function in Python
bindings to fix negative ref count of _global_py_pool object.
* subversion/bindings/swig/include/svn_types.swg
(%typemap(default) apr_pool_t *): Retrieve apr_pool_t * pointer from args
parameter only when _global_pool is NULL in order to increase correctly ref
count.
(%typemap(in) apr_pool_t *): Retrieve apr_pool_t * pointer from the Python
object and override _global_py_pool when an argument is not the last
apr_pool_t * argument.
(%typemap(in) apr_pool_t *scratch_pool): New typemap.
(%typemap(freearg) apr_pool_t *): Decreament ref count of Python object only
once.
* subversion/bindings/swig/python/tests/client.py
(test_inherited_props): Add tests which TypeError raises when passing non
apr_pool_t * object to function which has multiple apr_pool_t * arguments.
(test_conflict): Add tests that the _parent_pool attribute of the result
wrapper object doesn't point scratch pool object but points result pool
object.
Patch by: jun66j5
(tewaked by futatuki)
Index: subversion/bindings/swig/include/svn_types.swg
===================================================================
--- subversion/bindings/swig/include/svn_types.swg (revision 1877963)
+++ subversion/bindings/swig/include/svn_types.swg (working copy)
@@ -554,23 +554,55 @@
%typemap(default, noblock=1) apr_pool_t
*(apr_pool_t *_global_pool = NULL, PyObject *_global_py_pool = NULL)
{
- if (svn_swig_py_get_pool_arg(args, $descriptor,
- &_global_py_pool, &_global_pool))
+ if (_global_pool == NULL &&
+ svn_swig_py_get_pool_arg(args, $descriptor, &_global_py_pool,
+ &_global_pool))
SWIG_fail;
$1 = _global_pool;
}
%typemap(in, noblock=1) apr_pool_t * {
- /* Verify that the user supplied a valid pool */
- if ($input != Py_None && $input != _global_py_pool) {
- SWIG_Python_TypeError(SWIG_TypePrettyName($descriptor), $input);
- SWIG_arg_fail($svn_argnum);
- SWIG_fail;
- }
+ if ($input != Py_None && $input != _global_py_pool)
+ {
+ apr_pool_t *ptr;
+ if (svn_swig_py_convert_ptr($input, (void **)&ptr, $descriptor) == 0)
+ {
+ $1 = ptr;
+ }
+ else
+ {
+ SWIG_Python_TypeError(SWIG_TypePrettyName($descriptor), $input);
+ SWIG_arg_fail($svn_argnum);
+ SWIG_fail;
+ }
+ Py_DECREF(_global_py_pool);
+ _global_py_pool = $input;
+ Py_INCREF(_global_py_pool);
+ }
}
+/* scratch_pool is always set by "default" typemap, however type check
+ * is still needed.
+ */
+%typemap(in, noblock=1) apr_pool_t *scratch_pool {
+ if ($input != Py_None)
+ {
+ apr_pool_t *ptr;
+ if (svn_swig_py_convert_ptr($input, (void **)&ptr, $descriptor) != 0)
+ {
+ SWIG_Python_TypeError(SWIG_TypePrettyName($descriptor), $input);
+ SWIG_arg_fail($svn_argnum);
+ SWIG_fail;
+ }
+ }
+}
+
%typemap(freearg) apr_pool_t * {
- Py_XDECREF(_global_py_pool);
+ if (_global_py_pool != NULL)
+ {
+ Py_XDECREF(_global_py_pool);
+ _global_py_pool = NULL;
+ }
}
#endif
Index: subversion/bindings/swig/python/tests/client.py
===================================================================
--- subversion/bindings/swig/python/tests/client.py (revision 1877963)
+++ subversion/bindings/swig/python/tests/client.py (working copy)
@@ -477,6 +477,10 @@
def test_inherited_props(self):
"""Test inherited props"""
+ pool = core.Pool()
+ result_pool = core.Pool(pool)
+ scratch_pool = core.Pool(pool)
+
trunk_url = self.repos_uri + b'/trunk'
client.propset_remote(b'svn:global-ignores', b'*.q', trunk_url,
False, 12, {}, None, self.client_ctx)
@@ -483,6 +487,12 @@
head = core.svn_opt_revision_t()
head.kind = core.svn_opt_revision_head
+ self.assertRaises(TypeError, client.propget5, b'svn:global-ignores',
+ trunk_url, head, head, core.svn_depth_infinity, None,
+ self.client_ctx, 42)
+ self.assertRaises(TypeError, client.propget5, b'svn:global-ignores',
+ trunk_url, head, head, core.svn_depth_infinity, None,
+ self.client_ctx, None, 42)
props, iprops, rev = client.propget5(b'svn:global-ignores', trunk_url,
head, head, core.svn_depth_infinity,
None, self.client_ctx)
@@ -491,7 +501,8 @@
dir1_url = trunk_url + b'/dir1'
props, iprops, rev = client.propget5(b'svn:global-ignores', dir1_url,
head, head, core.svn_depth_infinity,
- None, self.client_ctx)
+ None, self.client_ctx, result_pool,
+ scratch_pool)
self.assertEqual(iprops[trunk_url], {b'svn:global-ignores':b'*.q\n'})
self.proplist_receiver_trunk_calls = 0
@@ -592,10 +603,15 @@
client.update4((path,), rev, core.svn_depth_unknown, True, False, False,
False, False, self.client_ctx)
- pool = core.Pool()
- conflict = client.conflict_get(trunk_path, self.client_ctx, pool)
+ # New pools, only for examination where the result comes from.
+ result_pool = core.Pool()
+ scratch_pool = core.Pool()
+ conflict = client.conflict_get(trunk_path, self.client_ctx,
+ result_pool, scratch_pool)
self.assertTrue(isinstance(conflict, client.svn_client_conflict_t))
+ self.assertNotEqual(conflict._parent_pool, scratch_pool)
+ self.assertEqual(conflict._parent_pool, result_pool)
conflict_opts = client.conflict_tree_get_resolution_options(conflict,
self.client_ctx)
@@ -602,8 +618,6 @@
self.assertTrue(isinstance(conflict_opts, list))
self.assert_all_instances_of(conflict_opts,
client.svn_client_conflict_option_t)
- pool.clear()
-
@unittest.skip("experimental API, not currently exposed")
def test_shelf(self):
"""Test shelf api."""
