GNOME Bugzilla – Bug 118536
Make g_signal_connect_object'ed handlers disconnect when the data object is destroyed
Last modified: 2012-10-08 15:35:49 UTC
It'd be quite useful to have signal handlers connected using g_signal_connect_object(instance, signal, handler, gobject, flags) be disconnected when the data gobject dies. From a discussion about this with yosh in #gtk+, it seems that when the data gobject dies, the closure built by g_signal_connect_object gets invalidated, but that each time the emiting instance emits the signal after the gobject dies, there'll be a small overhead incurred because the closure will be checked.
I think I can code up a patch doing this if people think it is a good idea. Would changing the behavior g_signal_connect_object be fine or making a new API be better? It's probably not used that much, but it strictly is a behavior change...
I consider this a fairly bad bug - there is a potential for unbounded memory leaks. I can't find an actual example of this occuring in the uses in GTK+ of g_signal_connect_object(), but the potential is there.
IIRC, in the original reporters case, it would be leaky, since it was many short-lived objects being attached/destroyed in succession. (I suggested using weakrefs as a workaround) So, Owen, do you think changing the behavior is appropriate?
IMO, it's not a "behavior change" - I don't think the current behavior is even detectable except by watching top. It's just a straightforward bug. I asked Tim about it and he said something along the lines of "yeah, I've been meaning to fix that for a long time".
Ok, simple straightforward implementation attached. Comments?
Created attachment 19571 [details] [review] Patch implementing fix
Created attachment 19756 [details] [review] Second implementation, fixes some bugs
One thing I can think of left to do is use a memory pool for allocation of InstanceInfo's, but I don't know if that really is a win or not.
Created attachment 19776 [details] [review] Better implementation
Owen said: I'd really like to get Tor's comments on this, and on implementability on Windows. Other than that, we should go ahead and add this... if I remembered what was going on at all, I'd look through the changes since thelast patch, but I don't at this point. Note jrb's comment about extraneous chunks in the patch Do we need GPid or something like that that is 'int' on UNIX anda process handle on Windows? GSpawn is already problematical, butwe shouldn't propagate the breakage. (And I assume passing handlesthrough ints will break for Win64...) This bug evolved into something different, so we may want to leave itopen after the patch is applied and move it to [future]
Sorry, wrong bug.
i don't like the idea of introducing a new InstanceInfo structure per handler (and if this was required, InstanceInfo should be made part of struct Handler). basically, a variant of handler_lookup() needs to be introduced, that can lookup a handler from an instance and a closure pointer. that's enough to identify a signal handler to be disconnected from a closure invalidation handler, it just needs the instance as data argument for that kind of setup. it probably wouldn't take me too long to hack something like this up. what's more tedious to do though, and what should be done first is implementing a test case that tests various connections, destroys instance and/or data objects and connects closures to multiple signal handlers. that test should expose the lazy closure finalization behaviour. based on that, a stable implementation is much easier to produce.
OK, it looks like we aren't going to get to this for 2.4.0. I've added docs: <para> Note that there this a bug in GObject that makes this function much less useful than it might seem otherwise. Once @gobject is disposed, the callback will no longer be called, but, the signal handler is <emphasis>not</emphasis> currently disconnected. If the @instance is itself being freed at the same time than this doesn't matter, since the signal will automatically be removed, but if @instance persists, then the signal handler will leak. You should not remove the signal yourself because in a future versions of GObject, the handler <emphasis>will</emphasis> automatically be disconnected. </para> <para> It's possible to work around this problem in a way that will continue to work with future versions of GObject by checking that the signal handler is still connected before disconnected it: <informalexample><programlisting> if (g_signal_handler_is_connected (instance, id)) g_signal_handler_disconnect (instance, id); </programlisting></informalexample> </para> Fixing this is actually going to be somewhat of an incompatible API change, but the breakage for apps that are already disconnecting the signal handler themselves (without the above workaround) is only an additional warning message.
Created attachment 28677 [details] signal tests Here is an attempt at a test suite for g_signal_connect_closure() It tests the current lazy closure finalization and will have to be adjusted for the new behaviour.
Created attachment 28946 [details] [review] new patch which avoids InstanceInfo Here is a reworked version of yoshs patch which avoids the need for an InstanceInfo struct. Instead, it modifies handler_lookup() to allow looking up by handler_id or by closure.
Man, the original patch here is coming up to 3 years old now, together with Owens comment "I consider this a fairly bad bug - there is a potential for unbounded memory leaks." Tim, any hope for getting this reviewed before 2.14 ?
Updating bug metadata. This still affects recent versions.
Ping.
Tim, still waiting for a review here...
If the thing blocking acceptance of this fix is the behaviour change, couldn't you at least add g_signal_connect_object_properly() (insert better name here) and deprecate the buggy one? (Telepathy developers have ended up reinventing this function as a local utility thing, which makes me unhappy.)
Any news on this, it is not possible to just have a function called g_object_disconnect_all then one can called in the _finalize function.
Ping? What's preventing this from going in?
For what it's worth, here's the regression test for equivalent functionality in telepathy-glib, which might have better coverage of some code paths: http://git.collabora.co.uk/?p=telepathy-glib.git;a=blob;f=tests/signal-connect-object.c;hb=master and here's the actual implementation: http://git.collabora.co.uk/?p=telepathy-glib.git;a=blob;f=telepathy-glib/util.c;hb=master
Applying the patch at this point will result in tonnes of g_critical() spew from code written by thousands of application developers that didn't read Owen's warning. I think it should happen anyway. The note in the docs is extraordinarily annoying. I only came to fully understand what it meant today and I've been avoiding using g_signal_conect_object() for years because of it. If the change is applied, the generated critical spew will be entirely harmless (I think) and we can point developers at the note that was added to the docs years ago to explain to them why it's their code that is broken. Meanwhile, people can stop doing it "the proper way" and the other people who were doing it in the other broken way (which now becomes "the right way") will suddenly have their apps using less memory.
So, it seems weveryone agrees that this should go in immediately after the next major release?
(In reply to comment #24) > If the change is applied, the generated critical spew will be entirely harmless > (I think) But what will cause the g_critical? Are we trying to unref() an object whose refcount is 0? If so, it's true that will most of the time be a critical, but it could easily be a fatal crash too (e.g. if libc malloc() is able to call sbrk() to shrink the data segment after we call free()).
No. The problem is this: obj = obj_new(); id = g_signal_connect_object (other_obj, "sig", G_CALLBACK (cb), obj, 0); g_object_unref (obj); Currently, the handler is in a connected state, but will never be delivered (due to obj no longer existing). It is still proper to disconnect it, though: g_signal_handler_disconnect (other_obj, id); After the patch, the signal handler will already be disconnected for you when 'obj' is disposed. Then it won't exist anymore, so your attempt to do the disconnect (as above) will give a critical about "no such signal handler exists". That's why the documentation has long instructed the use of this pattern when disconnecting signals connected with g_signal_connect_object(): (quoting from the docs) if (g_signal_handler_is_connected (instance, id)) g_signal_handler_disconnect (instance, id);
(In reply to comment #27) > > After the patch, the signal handler will already be disconnected for you when > 'obj' is disposed. Then it won't exist anymore, so your attempt to do the > disconnect (as above) will give a critical about "no such signal handler > exists". It looks like the signal handler ids are a sequentially incrementing static 'long' variable, so one would have to overflow that in order to get a failure other than a critical. Unlike the ids associated with GSource, the GSignal ids are process-global. I just wanted to verify though that one couldn't e.g. remove a different signal handler.
(In reply to comment #25) > So, it seems weveryone agrees that this should go in immediately after the next > major release? We've just had a major release. Can this happen yet? Would it help this to happen if someone adapted <http://cgit.freedesktop.org/telepathy/telepathy-glib/tree/tests/signal-connect-object.c> to provide (additional?) test coverage?
Master is wide open at this point, if we have an agreed-on patch
Created attachment 226049 [details] [review] [gsignal] disconnect invalidated closures Modify gsignal to automatically disconnect a GClosure that becomes invalid (in the g_closure_invalidate() sense). Previously, when g_signal_connect_object() was used with a GObject as the user_data and that object was destroyed, the handler would no longer be called but the signal handler was itself was not disconnected (ie: the bookkeeping data was kept around). The main effect of this patch is that these signal handlers will now be automatically disconnected (and fully freed). The documentation for g_signal_connect_object() has anticipated this change for over 10 years and has advised the following workaround when disconnecting signal handlers connected with g_signal_connect_object(): if (g_signal_handler_is_connected (instance, id)) g_signal_handler_disconnect (instance, id); If your code follows this practice then it will continue to work. If your code never disconnects the signal handler then it was wasting memory before (and this commit fixes that). If your code unconditionally disconnects the signal handler then you will start to see (harmless) g_critical() warnings about this and you should fix them.
Created attachment 226050 [details] [review] [gsignal] fix up a crasher in previous commit The previous commit introduced a new variable in the Handler struct but didn't initialise it. This was causing some tests to crash.
Created attachment 226051 [details] [review] fix g_signal_connect_object() documentation g_signal_connect_object() now works properly, so we can remove the note in the docs about it being broken.
Attachment 226049 [details] pushed as d03d26f - [gsignal] disconnect invalidated closures Attachment 226050 [details] pushed as c15769d - [gsignal] fix up a crasher in previous commit Attachment 226051 [details] pushed as 8fd7570 - fix g_signal_connect_object() documentation