GNOME Bugzilla – Bug 510511
finalize() not called for removed sources
Last modified: 2013-03-01 10:13:16 UTC
Calling a gobject.Source object's destroy() method leads to pyg_source_destroy(), which sets self->source to NULL, but does not unref the source before doing so. When the Python wrapper object is GCed, pyg_source_clear() would normally unref the source, but does not do so on destroy()ed objects because self->source is already NULL. Attached is a patch against r729 that fixes this memory leak.
Created attachment 103172 [details] [review] patch against r729
Comment on attachment 103172 [details] [review] patch against r729 >Index: gobject/pygsource.c >=================================================================== >--- gobject/pygsource.c (revision 729) >+++ gobject/pygsource.c (working copy) >@@ -116,6 +116,7 @@ > } > > g_source_destroy(self->source); g_source_destroy should already unref the source >+ g_source_unref(self->source); > self->source = NULL; > > Py_INCREF(Py_None);
Yes, but the GSource refcount is initially 1, and is then incremented by g_source_attach() when the source is attached to a context. The role of g_source_destroy() is to remove a source from its context; it decrements the refcount, but the count won't hit zero and be deallocated unless pygobject unrefs it again.
Created attachment 103183 [details] [review] bcs_destroy_leak.patch There are more issues here than addressed by the first patch; attaching an updated patch. I believe that this change fixes some substantial problems with GSource wrapper memory management. It's tested, but the lifetime issues here are messy. I'd appreciate a review.
Created attachment 103227 [details] source_lifecycle_tests.py A few test cases. I see failures on all but the last of these under r737.
Created attachment 103229 [details] [review] bcs_gsource_lifecycle.patch Updated patch against r737. This patch also disables applying the cyclic GC to Source objects, which is likely to be impossible given the existence of Source finalize() methods. Honestly, my guess is that there are still bugs in this implementation; the lifecycle issues are tricky, and I'm not confident that this patch addresses them all. I'm tempted to provide a more substantial rewrite of the GSource wrapper. Is the original author of this code still around?
Created attachment 106216 [details] [review] bcs_gsource_lifecycle.patch
Created attachment 106217 [details] [review] bcs_gsource_tests.patch Patch against gobject unit test suite: test wrapper memory management.
(In reply to comment #7) > Created an attachment (id=106216) [edit] > gsource memory management patch > It's pretty hard to review this patch. I would appreciate if you could write a patch which is not doing anything unnecessary, eg changing the indentation, variable declaration order, function ordering. Keep the neccessary changes to a minimum.
Comment on attachment 106217 [details] [review] bcs_gsource_tests.patch >Index: tests/test_source.py >+import types >+import collections These two imports are unused. >-from common import gobject >+#from common import gobject >+import gobject This change is not necessary, please leave it as it was since you need to import gobject from common for the testsuite to work. >+from trampoline import Trampoline Where is this module coming from? >+# update a list on weakref callback >+def on_weakref_dead(d, i = 0): >+ def callback(r): >+ d[i] = True >+ >+ return callback > class TestSource(unittest.TestCase): >+#class TestSource(object): Why commented out? Just remove the old definition.
(In reply to comment #9) > (In reply to comment #7) > > Created an attachment (id=106216) [edit] > > gsource memory management patch > > > > It's pretty hard to review this patch. I would appreciate if you could write a > patch which is not doing anything unnecessary, eg changing the indentation, > variable declaration order, function ordering. Keep the neccessary changes to a > minimum. You're right; I would very much like to improve style consistency here, but that's a separate change. Attaching a revised patch. To provide context, there are real problems with GSource wrapper memory management as it stands now: eg, finalize methods can cause segfaults and memory corruption, destroy()ed Source wrappers are always leaked, and the cyclic GC will segfault on any reference to a Timeout or Idle instance. I believe that this patch fixes the issues above, and handles the memory management situations which can reasonably be handled. It doesn't fix everything; there are source comments which explain in more detail.
Created attachment 106264 [details] [review] bcs_gsource_lifecycle.patch Removed changes which were purely cosmetic.
(In reply to comment #10) > (From update of attachment 106217 [details] [review] [edit]) > >+from trampoline import Trampoline > > Where is this module coming from? > > >+# update a list on weakref callback > >+def on_weakref_dead(d, i = 0): > >+ def callback(r): > >+ d[i] = True > >+ > >+ return callback > > > class TestSource(unittest.TestCase): > >+#class TestSource(object): > > Why commented out? Just remove the old definition. Cruft in there from my testing. (Oops---I didn't think anyone was actually paying attention to this bug yet.) Attaching a revised tests patch, as well as a patch to add the trampoline module. The trampoline-related code isn't strictly necessary, but it has served as a good stress test.
Created attachment 106268 [details] [review] bcs_gsource_tests.patch Clean up tests patch.
Created attachment 106269 [details] [review] bcs_trampoline.patch Add trampoline module for the testCoroutinesApp() test in test_gsource.py.
Created attachment 108142 [details] [review] bcs_gsource_lifecycle.patch Updated patch; fix destroy() of unattached sources.
Created attachment 108143 [details] [review] bcs_gsource_tests.patch Add corresponding test case.
I applied the patches, got some conflicts and this while running make check: .....python: Objects/object.c:1287: PyObject_GenericGetAttr: Assertion `mro != ((void *)0)' failed. 0447b799-e78a-3401-106658d1-347cf250 is dumped Could you update the patch to trunk? Btw, it would be easier if you put the test+trampoline+gobject changes in the same diff.
This bug is over 2 years old. Is it still relevant?
Glancing at git HEAD, my guess is that these issues have not yet been fixed. The associated test suite still segfaults with 2.21.1: $ python test_source.py -v testSourcePrepare (__main__.TestSource) ... ok testSources (__main__.TestSource) ... ok test504337 (__main__.TestTimeout) ... ok testDeallocAfterContextDeathAndGC (__main__.TestTimeout) ... ERROR testDeallocAfterDestroy (__main__.TestTimeout) ... ERROR testDeallocAfterReturnAndGC (__main__.TestTimeout) ... Segmentation fault I'll attach test_source.py. The GSource binding is in need of some love and attention, but I don't, unfortunately, have time to bring this patch up to date right now. IIRC, it also doesn't fix all of the lifecycle issues---in particular, those involving reference cycles that pass through callbacks. (I'm not sure if those are practically fixable.)
Created attachment 171038 [details] test_source.py
the bug still exists (gdb) bt full
+ Trace 225024
(gdb)
fabio@OptimusPrime:~$ python test1.py ...EESegmentation fault (core dumped) fabio@OptimusPrime:~$
Created attachment 211245 [details] test_source.py with GI (buggy) Just driving by here. Much has changed since pygobject went GI only. I made a quick attempt to port test_source.py to GI (attaching). I now get: ====================================================================== FAIL: testDeallocAfterContextDeathAndGC (__main__.TestTimeout) ---------------------------------------------------------------------- Traceback (most recent call last):
+ Trace 230005
assert d[0] AssertionError
assert d[0] != None AssertionError
which shows that this still does not seem to work quite right with GI. With the testDeallocAfterReturnAndGC case I get testDeallocAfterReturnAndGC (__main__.TestTimeout) ... Exception AttributeError: "'gi._glib.Source' object has no attribute 'finalize'" in 'garbage collection' ignored Fatal Python error: unexpected exception during garbage collection which shows that this test case would actually succeed, as it's trying to clean up the GSource object. That's the one that previously segfaulted, right? The other tests seem better off: testSourcePrepare (__main__.TestSource) ... ok testSources (__main__.TestSource) ... ok test504337 (__main__.TestTimeout) ... ok testDeallocAfterDestroy (__main__.TestTimeout) ... ok testDestroyWithoutAttachtestIdleBug504337 (__main__.TestTimeout) ... ok testLoopClosureCycle (__main__.TestTimeout) ... ok testStillAttached (__main__.TestTimeout) ... ok testTimeoutBug504337 (__main__.TestTimeout) ... ok (__main__.TestTimeout) ... ok So, leaving this open for further investigation, but removing the patch flag.
Current pygobject 3.7 removed most of the static GSource bindings in favour of using GI, so the affected code does not exist any more.
Actually, the patches are obsolete, but it seems sources are still not finalized. So reopening, and marking the patches as obsolete instead, sorry for the noise.
I fixed the reference leak in http://git.gnome.org/browse/pygobject/commit/?id=6f6c0ceff00f