[ Changing subject to get this unburried from a long semi-related
thread. More below. ]
On Tue, Dec 22, 2020 at 6:27 PM Yasuhito FUTATSUKI
<futat...@yf.bsdclub.org> wrote:
>
> On 2020/12/21 15:50, Daniel Shahaf wrote:
> > Johan Corveleyn wrote on Sun, 20 Dec 2020 23:05 +0100:
> >> 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?
> >
> > Not this Daniel :)
>
> Really? :)
>
> I know that Jun is not satisfied with my patch. Also, I don't think
> my patch really resolve the problems.
>
> For svn_repos_node_editor(), those programmer who want to pass different
> pools to the wrapper function should keep the reference for node_pool
> explicitly until the wrapper objects which contains tree structure
> allocated from node_pool.
>
> For other APIs which accept result_pool and scratch_pool, the problem
> of my patch is:
>
> (1) If a wrapper API accept a pure Python objects and construct some
> C struct object from them, wrapper function uses scratch_pool
> for their allocation.
> (2) If a C API correspoinding to such a wrapper returns some C
> structure which contain some object created in case (1) as a
> part of them, those result objects depends on scratch_pool
> as well as result_pool. However Python don't know such a
> dependency because there is no reference from wrapper object of
> result object to scratch_pool object.
>
> If object alocation in (1) uses result_pool instead of srcatch_pool,
> the problem in (2) will be resolved, and perhaps Jun wants to implement
> it. However in this case, temporary objects for C API call will left
> in result_pool even if they aren't parts of result objects.
>
> Any ways, I think this is a limitation of non-custom wrapper function.
> So it might be one of good solution that we never allow to pass
> multiple pools to Python wrapper function.
>
> Cheers,
> --
> Yasuhito FUTATSUKI <futat...@yf.bsclub.org>
Hi,
Just wanted to highlight that this issue with the Python bindings and
multiple apr_pool_t* arguments is not fixed yet. As I understand it,
it leads to a "negative refcount" assertion in debug mode, and in a
potential memory leak in both debug and release mode.
There were some patch iterations by Jun and Yasuhito, but apparently
the approach still has some problems (as explained by Yasuhito here
above). Attaching the latest patch from the original thread, to have
everything together.
To be clear: I'm not personally affected by this problem, and I don't
understand enough about it to help ... I just don't want this problem
/ these efforts to be lost in the mists of our mailinglists.
Hopefully, some people can take another look at this.
--
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."""
