|
On Friday, February 22, 2013 08:05:51 PM Daniel Veillard wrote: > On Fri, Feb 22, 2013 at 12:11:33AM -0800, Alexey Neyman wrote: > > [Adding back the list - I accidentally hit "reply" instead of "reply all" > > in previous email] > > > > On Friday, February 22, 2013 01:38:51 PM Daniel Veillard wrote: > > > [sending again since mail was apparently lost] > > > > > > On Thu, Feb 21, 2013 at 10:21:18AM -0800, Alexey Neyman wrote: > > > > On Thursday, February 21, 2013 04:01:45 PM you wrote: > > > > > On Wed, Feb 20, 2013 at 06:28:25PM -0800, Alexey Neyman wrote: > > > > > > Hi libxml developers, > > > > > > > > > > > > [BTW, is this list alive?] > > > > > > > > > > > yes, why ? > > > > > > > > I've sent another patch 3 days ago, and there was no response at all. > > > > Here > > > > it is: > > > > > > > > https://mail.gnome.org/archives/xml/2013-February/msg00013.html > > > > > > > yep, I saw it, but it's more complex, I'm worried about what happens > > > > > > if there is some kind of deallocation like calling popInputCallbacks() > > > from the python bindings themselves, while said input is still in use > > > by a parser, I smell some reference counting would have to be added to > > > take care of this, i.e. the single > > > > > > Py_INCREF(pythonInputOpenCallbackObject); > > > > > > on registration may not be sufficient to cover that case. The problem > > > is that the new use is done directly from C and there is no mechanism we > > > can use to increment/decrement the counter when a parser creates an > > > input based on those callbacks. > > > > I see your point... If I understand it correctly, I need to increment the > > refcount each time open callback succeeds (i.e. in > > pythonInputOpenCallback(), when the return value is not Py_None) and > > decrement it whenever the close callback is called (i.e., wrap > > xmlPythonFileCloseRaw() into a new function, pythonInputCloseCallback()). > > yes, that's it basically, we need to catch new open and closes and > adjust ref counts accordingly.
Actually, I reviewed it again and I don't think any additional refcounts are necessary:
- Python method (registerInputCallback) passes a Python object, findOpenCallback, to C code (to libxml2mod.xmlRegisterInputCallback) - C code adds a reference to findOpenCallback - Whenever an entity is loaded, Python object is called. It returns *another* Python object (file, StringIO, whatever) that will be used as 'context' for read/close callbacks, and PyObject_CallFunction creates a new reference on the returned object. - The read callback will use this newly returned Python object, and the close callback will drop the reference created by PyObject_CallFunction - releasing this object. - When there is no more Python callbacks registered, popInputCallbacks() will call libxml2mod.xmlUnregisterInputCallback() - which will drop the reference on findOpenCallback.
So, even if popInputCallbacks() is called in the middle of reading an already opened entity - it will only affect new attempts to load entity, but not already opened ones - which use other objects as 'context'.
I did find a minor bug in my code, though, where I didn't drop a reference on Py_None when PyObject_CallFunction returned it with a new reference. Since Py_None is a singleton and always exists, this bug shouldn't have had any runtime effect, though.
Updated patch, with a new test case, attached.
Also, I split the fixes to setEntityLoader() into a separate patch, also attached.
Regards, Alexey.
> > > Am I missing anything? > > no, hopefully that will be sufficient :-) > > > > So i'm afraid we are stuck with a known > > > problem we can't fix, unless we never release the python reference, > > > which creates a leak (but a leak is better than a crash) > > > > > > in any case tests suite addition would be welcome, yes :-) > > > > Yes, I just wanted to first settle on the API for the binding :) > > Having a test which exercise the above refcount small problem would > be a good thing too, > > thanks a lot ! > > Daniel |
Attachment:
input-cb-bindings.patch
Description: Text Data
Attachment:
fix-entity-loader.patch
Description: Text Data