pygobject / pgi

[Unmaintained: Use PyGObject instead] GTK+ / GObject Introspection Bindings for PyPy.
GNU Lesser General Public License v2.1
74 stars 16 forks source link

Unclear how to deal with custom finalizers #11

Closed pwaller closed 9 years ago

pwaller commented 10 years ago

I've found another case where the code generator doesn't look like it can do the right thing. In this case it's a List of TextAttributes which you have to free with poppler-page-free-text-attributes.

Unfortunately when the pgi-generated code calls t5.free() this misses all of the contents of the list which also need freeing.

Even more unfortunately, I can't see anywhere this is described in the gobject introspection bindings.

Any thoughts on how to solve this? I can't think of any way other than to implement an override for it which calls the correct free function.

# dependencies:
#   e4: <_FuncPtr object at 0xe74120>
#   e7: <class 'Poppler.TextAttributes'>
#   e2: <module '__builtin__' (built-in)>
#   e1: <module 'ctypes' from '/usr/lib/python2.7/ctypes/__init__.pyc'>
#   e9: <class 'ctypes.c_void_p'>
# backend: ctypes
def get_text_attributes(self):
    '''get_text_attributes() -> [Poppler.TextAttributes]'''

    # args: ['c_void_p']
    # ret: <class 'pgi.clib.glib.GListPtr'>
    t3 = self._obj
    t5 = e4(t3)
    t10 = []
    t11 = t5
    while t11:
        t12 = t11.contents
        t6 = e9(t12.data or 0).value
        if t6:
            t8 = e2.object.__new__(e7)
            t8._obj = t6
        else:
            t8 = None
        t10.append(t8)
        t11 = t12.next
    t5.free()
pwaller commented 10 years ago

Yeah. I don't find any reference to poppler_page_free_text_attributes which describes in a machine readable way that it should be used to destruct the return value of get_text_attributes.

So maybe this is a gobject introspection deficiency?

Or maybe not. Looking at the implementation of poppler_page_free_text_attributes it looks like all it does is call poppler_text_attributes_free, which is described in the bindings. Then I'm left with the mystery of why there is still a leak - maybe because we don't have any finalizer tracking object t6 from the code above?

I guess we should introduce something along the lines of what I did to correctly unref pages after allocation:

https://github.com/lazka/pgi/issues/8#issuecomment-50343821

https://github.com/lazka/pgi/blob/83a5a2df79ce4209d783d71472e4819192153dd7/pgi/codegen/ctypes_backend/types_interface.py#L113

But I'm not sure if this is the right approach.

lazka commented 10 years ago

The function is just a C helper:

poppler_page_free_text_attributes (GList *list)
{
  if (G_UNLIKELY (list == NULL))
    return;

  g_list_foreach (list, (GFunc)poppler_text_attributes_free, NULL);
  g_list_free (list);
}

See ownership transfer: https://developer.gnome.org/gi/unstable/gi-GIArgInfo.html#GITransfer

In this case, if it is GI_TRANSFER_EVERYTHING, we should manually free each item in the list and then free the list, like poppler_page_free_text_attributes does.

Thanks for your work btw, I'll try to look into it tomorrow.

pwaller commented 10 years ago

It is <return-value transfer-ownership="full">: https://gist.github.com/pwaller/ae4d48566347b6d23373#file-poppler-0-18-gir-xml-L3867

pwaller commented 9 years ago

Tidying up my personal issues list, so closing this. Please create a new issue if you're still interested in tracking it.