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>
