On Thu, May 15, 2008 at 12:52 PM, David Glasser
<glasser_at_davidglasser.net> wrote:
> So, let's say I write the following swig-py code:
>
> def foo():
> editor = MyEditor()
> e, eb = svn.delta.make_editor(editor)
> r, rb = svn.ra.do_update2(ra, ..., e, eb, ...)
> # do stuff with r, rb
>
> That's fine. Now let's say I do this:
>
> def DoUpdate(editor):
> e, eb = svn.delta.make_editor(editor)
> r, rb = svn.ra.do_update2(ra, ..., e, eb, ...)
> return r, rb
>
> def foo():
> editor = MyEditor()
> r, rb = DoUpdate(editor)
> # Do stuff with r, rb.
>
> This can lead to segfaults. Why? Because e and eb are destroyed at
> the end of DoUpdate, even though the C reporter in r and rb is holding
> onto references to it.
>
> In my particular code, I was actually wrapping r and rb in a Reporter
> class anyway, so I just added "reporter._hold_refs_ = [e, eb]".
> That's kind of hacky, though.
>
> I talked to epg about this for a while. Ideally what one would want
> would be for the do_update2 wrapper to do an incref on its e/eb
> arguments, and for something (__del__ on r, say) to decref them. This
> seems like it'll be a lot of work.
Will this be a lot of work? Maybe :) Here's a patch which implements
this solution. In theory this same approach could be used to make sure
all implicit scope rules are honored.
[[[
Teach the Python bindings to keep editors in scope for as long as the objects
which are built from the editor.
Suggested by: glasser
[ In subversion/bindings/swig ]
* python/tests/ra.py
(test_update): Test lifetime of editors.
* python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
(svn_swig_py_add_kid_to_pool): New function.
* include/proxy_apr.swg
(apr_pool_t.__init__, apr_pool_t.clear, apr_pool_t.destroy): Clear kids list.
(apr_pool_t._add_kid): New function.
* svn_delta.i
(py_editor, EDITOR): Update Python typemaps to save references to
the editor in a pool
so that we can be sure they live long enough.
]]]
The test case in this patch is pretty basic. I'd like to see a much
better test case before this gets committed (to test the implications
of this patch on functions other than update), but I don't have a lot
of time to work on this. Do you or epg have time to give this patch
some more rigorous tests?
Cheers,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-17 23:36:07 CEST