You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Johan Corveleyn <jc...@gmail.com> on 2020/12/20 22:05:38 UTC
Re: [PATCH] fix unable to load *.pyd files with Python 3.8.x on Windows
On Sat, May 23, 2020 at 2:17 PM Yasuhito FUTATSUKI <fu...@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
Python bindings issue with multiple apr_pool_t* arguments (was:
[PATCH] fix unable to load *.pyd files with Python 3.8.x on Windows)
Posted by Johan Corveleyn <jc...@gmail.com>.
[ 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
<fu...@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 <fu...@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 <fu...@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
Re: [PATCH] fix unable to load *.pyd files with Python 3.8.x on
Windows
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
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 <fu...@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 <fu...@yf.bsclub.org>
Re: [PATCH] fix unable to load *.pyd files with Python 3.8.x on
Windows
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Johan Corveleyn wrote on Sun, 20 Dec 2020 23:05 +0100:
> On Sat, May 23, 2020 at 2:17 PM Yasuhito FUTATSUKI <fu...@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 :)