After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 118536 - Make g_signal_connect_object'ed handlers disconnect when the data object is destroyed
Make g_signal_connect_object'ed handlers disconnect when the data object is d...
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
2.12.x
Other All
: High major
: ---
Assigned To: Tim Janik
gtkdev
Depends on:
Blocks:
 
 
Reported: 2003-07-29 01:29 UTC by Mariano Suárez-Alvarez
Modified: 2012-10-08 15:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch implementing fix (3.95 KB, patch)
2003-08-28 03:23 UTC, Manish Singh
none Details | Review
Second implementation, fixes some bugs (4.50 KB, patch)
2003-09-05 02:23 UTC, Manish Singh
none Details | Review
Better implementation (4.48 KB, patch)
2003-09-05 22:36 UTC, Manish Singh
none Details | Review
signal tests (8.62 KB, text/plain)
2004-06-14 05:12 UTC, Matthias Clasen
  Details
new patch which avoids InstanceInfo (6.12 KB, patch)
2004-06-23 05:49 UTC, Matthias Clasen
none Details | Review
[gsignal] disconnect invalidated closures (7.57 KB, patch)
2012-10-08 15:34 UTC, Allison Karlitskaya (desrt)
committed Details | Review
[gsignal] fix up a crasher in previous commit (809 bytes, patch)
2012-10-08 15:35 UTC, Allison Karlitskaya (desrt)
committed Details | Review
fix g_signal_connect_object() documentation (2.05 KB, patch)
2012-10-08 15:35 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Mariano Suárez-Alvarez 2003-07-29 01:29:51 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.
Comment 1 Manish Singh 2003-07-29 01:58:53 UTC
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...
Comment 2 Owen Taylor 2003-08-25 15:10:49 UTC
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.
Comment 3 Manish Singh 2003-08-25 15:19:47 UTC
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?
Comment 4 Owen Taylor 2003-08-25 18:48:46 UTC
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".
Comment 5 Manish Singh 2003-08-28 03:22:15 UTC
Ok, simple straightforward implementation attached. Comments?
Comment 6 Manish Singh 2003-08-28 03:23:13 UTC
Created attachment 19571 [details] [review]
Patch implementing fix
Comment 7 Manish Singh 2003-09-05 02:23:10 UTC
Created attachment 19756 [details] [review]
Second implementation, fixes some bugs
Comment 8 Manish Singh 2003-09-05 02:27:19 UTC
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.
Comment 9 Manish Singh 2003-09-05 22:36:29 UTC
Created attachment 19776 [details] [review]
Better implementation
Comment 10 Matthias Clasen 2004-01-22 00:29:50 UTC
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]
Comment 11 Matthias Clasen 2004-01-22 00:41:13 UTC
Sorry, wrong bug.
Comment 12 Tim Janik 2004-02-19 16:06:27 UTC
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.
Comment 13 Owen Taylor 2004-03-14 16:39:08 UTC
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.
Comment 14 Matthias Clasen 2004-06-14 05:12:59 UTC
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.
Comment 15 Matthias Clasen 2004-06-23 05:49:24 UTC
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.
Comment 16 Matthias Clasen 2006-07-22 17:43:08 UTC
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 ?
Comment 17 Christian Neumair 2006-12-25 15:47:28 UTC
Updating bug metadata. This still affects recent versions.
Comment 18 Christian Neumair 2008-02-27 13:10:10 UTC
Ping.
Comment 19 Matthias Clasen 2008-11-29 03:28:34 UTC
Tim, still waiting for a review here...
Comment 20 Simon McVittie 2009-04-08 12:49:34 UTC
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.)
Comment 21 Ali Abdallah 2009-04-08 14:09:28 UTC
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.
Comment 22 Arun Raghavan 2011-02-07 16:52:42 UTC
Ping? What's preventing this from going in?
Comment 23 Simon McVittie 2011-02-07 17:24:15 UTC
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
Comment 24 Allison Karlitskaya (desrt) 2011-04-13 05:12:30 UTC
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.
Comment 25 Benjamin Otte (Company) 2012-03-18 22:55:02 UTC
So, it seems weveryone agrees that this should go in immediately after the next major release?
Comment 26 Colin Walters 2012-03-20 13:51:31 UTC
(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()).
Comment 27 Allison Karlitskaya (desrt) 2012-03-20 14:58:26 UTC
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);
Comment 28 Colin Walters 2012-03-21 17:47:30 UTC
(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.
Comment 29 Simon McVittie 2012-10-04 17:29:18 UTC
(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?
Comment 30 Matthias Clasen 2012-10-04 20:31:10 UTC
Master is wide open at this point, if we have an agreed-on patch
Comment 31 Allison Karlitskaya (desrt) 2012-10-08 15:34:54 UTC
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.
Comment 32 Allison Karlitskaya (desrt) 2012-10-08 15:35:03 UTC
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.
Comment 33 Allison Karlitskaya (desrt) 2012-10-08 15:35:08 UTC
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.
Comment 34 Allison Karlitskaya (desrt) 2012-10-08 15:35:39 UTC
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