Re: [PATCH/RFC] Fix refcount on some Python callbacks

[Martin von Gagern <Ma...@gmx.net>]

Jelmer Vernooij wrote:
> The attached patch fixes the reference counting in the Python part of %
> callback_typemap. At the moment the reference counter of the Python
> function is not increased when passing it as a baton. This means that if
> Subversion is the only one still referencing the function it will be
> garbage collected, causing Subversion to segfault when it tries to call
> the Python callback. 
> 
> However, this patch doesn't decrement the reference counter once the
> callback is no longer used since there is no place to free batons as far
> as I know. That means we'll potentially keep Python methods around in
> memory that aren't used. Is that a problem or is there perhaps some way
> to work around this?

Hi!

I'd like to warm up this old thread again, as I have a new suggestion.
Generally you have some python function, pass it to some interface code 
that builds a wrapper around it to turn it into a c callback type, and 
wraps that internal c thing back into a python object. Correct so far?

Now the only thing needed in order to get clean garbage collection would 
be to attach the python function to the generated python object on the 
python level. I've got a demo for this in one instance:

provider = svn_auth_get_ssl_server_trust_prompt_provider(callback)
provider.callback_reference = callback

So the subversion bindings see provider as the swig wrapper and can use 
that to access the wrapped callback, just as before. Python, on the 
other hand, sees that the object provider holds a reference to the 
function callback, so it can't garbage collect the latter as long as the 
provider is live.

This is a simple example and executed in one bzr-svn branch:
https://code.launchpad.net/~gagern/bzr-svn/bug238529
related to https://bugs.launchpad.net/bugs/238529

I'm no proficient enough in using swig to formulate this yet, but I 
would assume that this kind of approach should work for all cases of 
functions that take one python object and return another python object 
that internally references the former but doesn't do so in python yet.

So instead of addressing this in each and every application using the 
python bindings, it would be preferable to solve this on the subversion 
side. Do you think this can be done? Is someone willing to do so? Or if 
not, maybe willing to point me or others into the right direction?

Greetings,
  Martin von Gagern

Re: [PATCH/RFC] Fix refcount on some Python callbacks

[Martin von Gagern <Ma...@gmx.net>]

David Glasser wrote:
> David James posted a patch to help with something similar (editors,
> not callbacks):

Comparing my approach to his, it looks to me like he would add 
references to the pools while I would have added those references to 
python objects. I guess the former more closely resembles the native 
workings of C with APR, while the latter is more in the Python style.

> I didn't really feel qualified to know if it was the right solution
> though, and ended up dropping it.

I'm a newcomer to Python, have never really worked with APR, so I 
wouldn't be "qualified" in any way either. On the other hand, I would 
assume that most attempts at a fix, even if not in the best of styles, 
would be better than the segmentation faults that occur now.

Greetings,
  Martin

Re: [PATCH/RFC] Fix refcount on some Python callbacks

[David Glasser <gl...@davidglasser.net>]

On Mon, Jun 9, 2008 at 11:53 AM, Martin von Gagern
<Ma...@gmx.net> wrote:
> Jelmer Vernooij wrote:
>>
>> The attached patch fixes the reference counting in the Python part of %
>> callback_typemap. At the moment the reference counter of the Python
>> function is not increased when passing it as a baton. This means that if
>> Subversion is the only one still referencing the function it will be
>> garbage collected, causing Subversion to segfault when it tries to call
>> the Python callback.
>> However, this patch doesn't decrement the reference counter once the
>> callback is no longer used since there is no place to free batons as far
>> as I know. That means we'll potentially keep Python methods around in
>> memory that aren't used. Is that a problem or is there perhaps some way
>> to work around this?
>
> Hi!
>
> I'd like to warm up this old thread again, as I have a new suggestion.
> Generally you have some python function, pass it to some interface code that
> builds a wrapper around it to turn it into a c callback type, and wraps that
> internal c thing back into a python object. Correct so far?
>
> Now the only thing needed in order to get clean garbage collection would be
> to attach the python function to the generated python object on the python
> level. I've got a demo for this in one instance:
>
> provider = svn_auth_get_ssl_server_trust_prompt_provider(callback)
> provider.callback_reference = callback
>
> So the subversion bindings see provider as the swig wrapper and can use that
> to access the wrapped callback, just as before. Python, on the other hand,
> sees that the object provider holds a reference to the function callback, so
> it can't garbage collect the latter as long as the provider is live.
>
> This is a simple example and executed in one bzr-svn branch:
> https://code.launchpad.net/~gagern/bzr-svn/bug238529
> related to https://bugs.launchpad.net/bugs/238529
>
> I'm no proficient enough in using swig to formulate this yet, but I would
> assume that this kind of approach should work for all cases of functions
> that take one python object and return another python object that internally
> references the former but doesn't do so in python yet.
>
> So instead of addressing this in each and every application using the python
> bindings, it would be preferable to solve this on the subversion side. Do
> you think this can be done? Is someone willing to do so? Or if not, maybe
> willing to point me or others into the right direction?

This is definitely a real problem.

David James posted a patch to help with something similar (editors,
not callbacks):

http://svn.haxx.se/dev/archive-2008-05/0738.shtml

I didn't really feel qualified to know if it was the right solution
though, and ended up dropping it.

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org