GNOME Bugzilla – Bug 583909
drop sinkfuncs by using the new stuff in glib instead
Last modified: 2010-06-23 07:01:32 UTC
Now that we have floating reference support in glib, we don't need for bindings to register their own sinkfuncs. This is needed by pybank as the bindings are generated at runtime based on the information available via introspection.
Created attachment 135388 [details] [review] drop sinkfuncs
What happens if someone still registers a sinkfunc? Bindings won't start leaking like mad once their sinkfuncs are just ignored?
(In reply to comment #2) > What happens if someone still registers a sinkfunc? Bindings won't start > leaking like mad once their sinkfuncs are just ignored? Well, the generic implementation proposed in the patch should be equivalent to whatever the bindings were doing in the sinkfuncs, though nothing prevented bindings of doing something else completely different in those.
Your patch doesn't seem to be completely valid. As I understand, objects can have a floating reference even if they don't inherit GInitiallyUnowned, even though most/all probably do inherit. In any case, just calling g_object_ref_sink() should be sufficient judging by documentation, i.e. all those ifs are not needed.
Created attachment 135496 [details] [review] addresses Paul's concern
(In reply to comment #4) > Your patch doesn't seem to be completely valid. As I understand, objects can > have a floating reference even if they don't inherit GInitiallyUnowned, even > though most/all probably do inherit. In any case, just calling > g_object_ref_sink() should be sufficient judging by documentation, i.e. all > those ifs are not needed. You are right, just calling g_object_ref_sink seems to be enough.
I tried to apply the patch. After it, several testcases start to fail. I'm not sure what is wrong, but please do run 'make check' before submitting a patch.
Created attachment 135661 [details] [review] passes the tests This one does pass the tests, sorry about that. You can see that we aren't unconditionally calling g_object_ref_sink(obj) because that would cause an unwanted extra reference in some cases.
Functions sink_gtkwindow() and sink_gtkinvisible() in PyGTK do something special. I don't understand what they do and I don't quite feel like removing that: might well break something or cause leaks.
Any idea about how to verify that? Any test we could add? There are already tests that verify reference counting and the last patch passes them, both in pygtk and pygobject. Also, if you think the approach is wrong and you can think of a better alternative, please share it.
The sink_gtkwindow() and sink_gtkinvisible() functions are there because the pointer returned is a borrowed reference and not a new reference. Without them, the Window / Invisible object would never be freed. I don't know if the generic sink function handles this or not; the last time I looked at this was when I ran into the bug that these functions fix.
(In reply to comment #11) > The sink_gtkwindow() and sink_gtkinvisible() functions are there because the > pointer returned is a borrowed reference and not a new reference. Without > them, the Window / Invisible object would never be freed. I don't know if the > generic sink function handles this or not; the last time I looked at this was > when I ran into the bug that these functions fix. The generic sink function is there to handle this. If it didn't do so, tests wouldn't pass.
Do the tests cover this case? I don't recall writing tests when the custom sink functions went in years ago (my bad). Note that I'm not opposed to dropping the custom functions (it's probably a good idea actually), I was just trying to explain why they were added. I'd like to say I'll look at it, but I don't have the spare cycles to do so right now.
(In reply to comment #13) > Do the tests cover this case? I don't recall writing tests when the custom > sink functions went in years ago (my bad). The patch before the last failed in some tests because of refcounting, though a quick grep didn't showed anything specific to sinkfuncs. When I get some time I will apply the old patch and check that the tests that fail are sufficient.
I don't think we can add a test to check that gtk.Window() is sunk after creation without wrapping GObject.is_floating(). I think it's correct to ask for evidence in such a low level issue, but then someone needs to actually look at it when presented. If the patch is read as well as the implementations of g_object_is_floating, G_IS_INITIALLY_UNOWNED and g_object_ref_sink, it will be clear that this patch is safe and correct. Also, if you play a bit around the pygobject and pygtk test suites, you will see that they won't pass if you don't either register the sinkfuncs or apply this patch. For now, I'm going to workaround this issue in Pygi by registering a dummy sinkfunc, but I think that for the sake of keeping pygobject updated, these issues should be given some consideration.
Created attachment 161366 [details] [review] [PATCH] Drop sinkfuncs. Fixes bug #583909 gobject/pygobject.c | 42 ++++++++---------------------------------- gobject/pygobject.h | 1 + 2 files changed, 9 insertions(+), 34 deletions(-)
New patch moves the call to pygobject_sink back to pygobject_register_wrapper_full since there are a couple of code paths for wrapping a gobject, all of which call pygobject_register_wrapper_full. It should be noted that PyGtk's use of special cases for GtkWindow and invisible objects are not needed since the combination of both G_IS_INITIALLY_UNOWNED(obj) and g_object_is_floating(obj) covers all bases. In the case of GtkWindow G_IS_INITIALLY_UNOWNED(obj) returns true and g_object_is_floating returns false but since we compare the values using or in the conditional we catch that case.
Sorry, git bz closed this ticket because another commit referenced it with the full url.
Comment on attachment 135661 [details] [review] passes the tests I'm unmarking this patch as obsolete because this actually passes the tests.
Review of attachment 161366 [details] [review]: This is not passing the tests here: ......................F.............F..F...........F......................................................................................................................................................... ====================================================================== FAIL: testErrors (test_properties.TestProperty) ---------------------------------------------------------------------- Traceback (most recent call last):
+ Trace 222300
self.assertRaises(TypeError, gobject.property, type=GEnum)
====================================================================== FAIL: testGCCollection (test_subtype.TestSubType) ---------------------------------------------------------------------- Traceback (most recent call last): File "../tests/test_subtype.py", line 146, in testGCCollection self.assertEqual(aref(), None) AssertionError: <gtk.Button object at 0x91a9c0c (GtkButton at 0x9031090)> != None ====================================================================== FAIL: testGObjectUnref (test_subtype.TestSubType) ---------------------------------------------------------------------- Traceback (most recent call last): File "../tests/test_subtype.py", line 134, in testGObjectUnref self.assertEqual(bref(), None) AssertionError: <gtk.Button object at 0xb757193c (GtkButton at 0x9031180)> != None ====================================================================== FAIL: testWeakRefCallback (test_subtype.TestSubType) ---------------------------------------------------------------------- Traceback (most recent call last): File "../tests/test_subtype.py", line 181, in testWeakRefCallback self.assertEqual(self._wr_args, (1, 2, 3)) AssertionError: None != (1, 2, 3) ---------------------------------------------------------------------- Ran 205 tests in 13.424s FAILED (failures=4)
I'm getting segmentation faults with make check from a clean build
With the patch applied and removing the gio tests which for some reason are crashing me, my patch passes make check
Maybe we should enter a ticket about your make check failures in a clean build, and once we have that fixed, add to this ticket which errors you get with my patch?
Created attachment 163220 [details] [review] Drop sinkfuncs. Fixes bug #583909 * use g_object methods to sink floating refs instead of allowing custom sink functions to be registered * we now sink inside of pygobject_new_full to handle cases where a library creates its own gobject via g_object_new and just needs a python wrapper - a previous patch had done the sink when creating the gobject, since it needs to call pygobject_new_full to wrap the object, this patch handles both cases (e.g. pygobject created object and externally created gobject)
Review of attachment 163220 [details] [review]: It's failing the pygtk tests :( .................................................E.............E...... GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed aborting... make[2]: *** [check-local] Trace/breakpoint trap (core dumped)
(In reply to comment #25) > Review of attachment 163220 [details] [review]: > > It's failing the pygtk tests :( > > .................................................E.............E...... > GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' > failed > aborting... > make[2]: *** [check-local] Trace/breakpoint trap (core dumped) Looks like a missing _ref() somewhere:
+ Trace 222335
We miss a _ref() for python subclasses of GtkWindow and GtkContainer, different codepath?
Applying this to your patch makes all tests I have to pass: diff --git a/gobject/gobjectmodule.c b/gobject/gobjectmodule.c index 69f3597..50a577c 100644 --- a/gobject/gobjectmodule.c +++ b/gobject/gobjectmodule.c @@ -2258,9 +2258,9 @@ pygobject_constructv(PyGObject *self, pygobject_init_wrapper_set(NULL); if (self->obj == NULL) { self->obj = obj; - pygobject_sink(obj); pygobject_register_wrapper((PyObject *) self); } + pygobject_sink(obj); } else { int i; for (i = 0; i < n_parameters; ++i) The idea is that for python subclasses, self->obj won't be NULL anymore after g_object_newv but we also want to sink those.
Ah, ok, I approve the change. If you approve the rest of the patch I say ship it!!!
This breaks modules depending on pygobject and that were using the sink functions (namely gst-python, see bug #621632) Could you re-instate (in addition) the previous behaviour so all those modules don't just break ? Marking that API as deprecated would also give dependent modules time to notice there will be a change and accordingly update to use the gobject way.
Created attachment 164223 [details] [review] Fall back to use the floating references API in glib if there isn't a sinkfunc defined. * tests/*: Add ref counting tests for floating objects * gobject/gobjectmodule.c, gobject/pygobject.c: Fall back to g_object_ref_sink or g_object_ref if there isn't a sinkfunc defined. Make sure that pygobject_sink gets called only once per GObject instance.
Review of attachment 164223 [details] [review]: Rest looks good to me, as long as gst-python keeps on working. ::: gobject/pygobject.c @@ +897,3 @@ * pygobject_new_full: * @obj: a GObject instance. + * @sink: whether to sink any floating reference found on the GObject. DEPRECATED. Warn if someone provides the sink func to get gst-python etc migrated.
Attachment 164223 [details] pushed as 00a85f6 - Fall back to use the floating references API in glib if there isn't a sinkfunc defined.
*** Bug 622436 has been marked as a duplicate of this bug. ***