On Sat, May 23, 2020 at 2:17 PM Yasuhito FUTATSUKI <futat...@poem.co.jp> wrote:
>
> On 2020/05/23 12:16, Daniel Shahaf wrote:
> > Yasuhito FUTATSUKI wrote on Sat, 23 May 2020 11:08 +0900:
> >> 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.
> >
> > What about svn_repos_node_editor()? It has two pools, and if I'm
> > reading the docstring correctly, "pool" will be used both for temporary
> > allocations and for allocating *editor and *edit_baton, while
> > "node_pool" will be used to allocate the tree that function constructs.
> >
> > That function is from 1.0 [according to its docstring], so it precedes
> > the result_pool/scratch_pool convention by several years.
>
> We can't add reference of "node_pool" Python object in apr_pool_t *
> members of the *editor and the *edit_baton C structure, at least
> with current wrapper mechanism. I think all we can do is to make
> "_parent_pool" attribute of wrapper objects for *editor and
> *edit_baton point to "pool" object.
>
> Fortunately as apr_pool_t *node_pool is used this API only, we can
> add typemap for it safely, in this case.
Now that the "unable to load *.pyd files" is fixed on trunk in
r1884642 (and nominated for backport to 1.14.x), I've retried this
latest patch for fixing the refcount issue. This patch makes the
swig-python tests succeed for me (on Windows with Python 3.9.1, in
debug configuration).
So, as far as I'm concerned, this is good to go, and certainly a step
forward again :-).
I don't feel up to doing a proper review though, it that's what's asked here.
Can Jun or Daniel perhaps take another look?
--
Johan
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.
(%typemap(in) apr_pool_t *node_pool): New typemap, only for
svn_repos_node_editor().
* 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,75 @@
%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;
+ }
+ }
+}
+
+/* A special typemap for svn_repos_node_editor(). In this API, parent pool
+ * object of the return values should be "pool" object.
+ */
+%typemap(in, noblock=1) apr_pool_t *node_pool {
+ 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;
+ }
+ }
+}
+
%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."""
