You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by Jun Omae <ju...@gmail.com> on 2021/05/03 09:06:13 UTC

Re: Should 'svn help' with APR lifetime debug enabled run without issues?

Hi,

On Wed, Apr 28, 2021 at 5:39 PM Rick van der Zwet
<in...@rickvanderzwet.nl> wrote:
> > Ack, but the backtrace does go through Subversion's swig-py bindings and
> > libsvn_fs_fs, so we might be involved nevertheless.
>
> ...
>
> error which seems to be related to the Trac code, I filled it over
> there: https://trac.edgewall.org/ticket/13401

The root cause is that sub pool is doubly destroyed because weakref's
callback is
not invoked when the pools are finalized by cyclic garbage collector
in Python 3.

See also https://bugs.python.org/issue40312

Proposed patch is attached.

[[[
swig-py: Fix doubly destroying memory pool because weakref's callback is not
invoked when it is finalized by cyclic garbage collector.

* subversion/bindings/swig/include/proxy_apr.swg
  (apr_pool_t.valid): Check whether parent pool is valid.

* subversion/bindings/swig/python/tests/pool.py
  (PoolTestCase): Add tests for pools referred from circular reference.
}}}

--
Jun Omae <ju...@gmail.com> (大前 潤)

Re: Should 'svn help' with APR lifetime debug enabled run without issues?

Posted by Jun Omae <ju...@gmail.com>.
On Tue, May 4, 2021 at 1:20 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Yasuhito FUTATSUKI wrote on Mon, 03 May 2021 15:12 +00:00:
> > The patch looks good to me. Also I confirmed that the test crash before
> > apply patch to proxy_apr.swg with signal 10 or signal 11 and that
> > the test passed without crash on Python 3.7 and 3.9, on FreeBSD 12.
> >
> > On Python 2.7, the test passed without crash both with/without patch
> > to proxy_apr.swg, as it would be expected.
> >
> > +1 to commit.
>
> Suggest to link to the Python bug ticket in the log message.
>
> Also, suggest to generate future patches with hunk titles enabled («svn
> diff -x-p»).  That tends to improve reviewability.

Thanks for the testing and suggestions.
Committed in r1889487.

-- 
Jun Omae <ju...@gmail.com> (大前 潤)

Re: Should 'svn help' with APR lifetime debug enabled run without issues?

Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Mon, 03 May 2021 15:12 +00:00:
> On 2021/05/03 18:06, Jun Omae wrote:
> > Hi,
> > 
> > On Wed, Apr 28, 2021 at 5:39 PM Rick van der Zwet
> > <in...@rickvanderzwet.nl> wrote:
> >>> Ack, but the backtrace does go through Subversion's swig-py bindings and
> >>> libsvn_fs_fs, so we might be involved nevertheless.
> >>
> >> ...
> >>
> >> error which seems to be related to the Trac code, I filled it over
> >> there: https://trac.edgewall.org/ticket/13401
> > 
> > The root cause is that sub pool is doubly destroyed because weakref's
> > callback is
> > not invoked when the pools are finalized by cyclic garbage collector
> > in Python 3.
> > 
> > See also https://bugs.python.org/issue40312
> > 
> > Proposed patch is attached.
> > 
> > [[[
> > swig-py: Fix doubly destroying memory pool because weakref's callback is not
> > invoked when it is finalized by cyclic garbage collector.
> > 
> > * subversion/bindings/swig/include/proxy_apr.swg
> >   (apr_pool_t.valid): Check whether parent pool is valid.
> > 
> > * subversion/bindings/swig/python/tests/pool.py
> >   (PoolTestCase): Add tests for pools referred from circular reference.
> > }}}
> 
> The patch looks good to me. Also I confirmed that the test crash before
> apply patch to proxy_apr.swg with signal 10 or signal 11 and that
> the test passed without crash on Python 3.7 and 3.9, on FreeBSD 12.
> 
> On Python 2.7, the test passed without crash both with/without patch
> to proxy_apr.swg, as it would be expected.
> 
> +1 to commit.

Suggest to link to the Python bug ticket in the log message.

Also, suggest to generate future patches with hunk titles enabled («svn
diff -x-p»).  That tends to improve reviewability.

Cheers,

Daniel

Re: Should 'svn help' with APR lifetime debug enabled run without issues?

Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2021/05/03 18:06, Jun Omae wrote:
> Hi,
> 
> On Wed, Apr 28, 2021 at 5:39 PM Rick van der Zwet
> <in...@rickvanderzwet.nl> wrote:
>>> Ack, but the backtrace does go through Subversion's swig-py bindings and
>>> libsvn_fs_fs, so we might be involved nevertheless.
>>
>> ...
>>
>> error which seems to be related to the Trac code, I filled it over
>> there: https://trac.edgewall.org/ticket/13401
> 
> The root cause is that sub pool is doubly destroyed because weakref's
> callback is
> not invoked when the pools are finalized by cyclic garbage collector
> in Python 3.
> 
> See also https://bugs.python.org/issue40312
> 
> Proposed patch is attached.
> 
> [[[
> swig-py: Fix doubly destroying memory pool because weakref's callback is not
> invoked when it is finalized by cyclic garbage collector.
> 
> * subversion/bindings/swig/include/proxy_apr.swg
>   (apr_pool_t.valid): Check whether parent pool is valid.
> 
> * subversion/bindings/swig/python/tests/pool.py
>   (PoolTestCase): Add tests for pools referred from circular reference.
> }}}

The patch looks good to me. Also I confirmed that the test crash before
apply patch to proxy_apr.swg with signal 10 or signal 11 and that
the test passed without crash on Python 3.7 and 3.9, on FreeBSD 12.

On Python 2.7, the test passed without crash both with/without patch
to proxy_apr.swg, as it would be expected.

+1 to commit.

Cheers,
-- 
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>