GNOME Bugzilla – Bug 667145
g_signal_connect_object thread safety
Last modified: 2018-05-24 13:38:41 UTC
With bug 548954 out of the way, we can talk about improving the safety of g_signal_connect_object() with respect to delivering signals to objects that are being unreffed in other threads. If we applied the patch in bug 118536 then we'd get some extra safety. As it is, the disposing of the object in the other thread results in the qdata being cleared up, resulting in invalidation of the GClosure. The patch means that the GClosure invalidation would unref the signal handler -- which is synchronised with a lock. Unfortunately, this doesn't help a whole lot because that lock is dropped just before invoking the closure. It's possible that the dropping of that lock would actually result in waking the other thread to finish up its finalization of the object. Without the patch, we're even worse off -- there's no synchronisation at all in the closure itself. We should probably move the GObject closures over to using the two-pass weak referencing approach: GWeakRef for safety and the callback version (or qdata, as now) for cleaning up afterwards.
GClosure vs. threads is some kind of a horrible nightmare. In g_closure_invoke(), for example, we see this: { bool in_marshal = closure->in_marshal; closure->in_marshal = TRUE; ... do the actual invoke... ... closure->in_marshal = in_marshal; } Ignore for a moment the unsafe bitfield access and consider two threads invoking a closure at the same time: thread1 thread2 take old value (FALSE) set to TRUE take old value (TRUE) invoke restore value to FALSE set to TRUE invoke restore value to TRUE at which point, in_marshal will be true forever and the marshal guards will never be called again...
Created attachment 204474 [details] [review] GObject closures: make threadsafe Here's an idea of what a patch might look like. It uses the metamarshal functionality of GClosure instead of the pre/post hooks (which cause the above-mentioned trouble with threads). This would allow us to deprecate the following APIs: - g_object_watch_closure - g_add_marshal_guards and the (non-trivial) supporting infrastructure. Note that I had to make a tweak in gsignal.c. g_signal_handlers_disconnect_by_func() only matches funcs inside of closures that lack metamarshallers, so we have to remove that check if we're going to expect _disconnect_by_func() to continue working with _connect_object(). All in all, I'm not totally thrilled with how this turned out, but aside from turning GClosure upside down (or nuking it entirely), I'm not sure if there's a better way.
Turning GClosure upside down might be an idea...short of that, this seems to be a step in the right direction. Not sure if the signal handler matching change is compatible enough, though. Maybe we need to check specifically for meta_marshal == 0 || meta_marshal == your meta marshaller ?
The problem with the current way GClosure works that most ties our hands here is the C marshallers. They access the ->data pointer of the closure directly. That's the crux of why we need to use the metamarshaller in order to ensure that the reference is strongly owned by the time the real marshaller is called. We could possibly modify glib-genmarshal to generate marshallers that do the work of the metamarshaller for themselves, but those marshallers are generic with respect to the user_data parameter -- it may not be a GObject. We could use a flag on the closure to mark that the user_data is a GObject, but GClosure is already a mess of bitfields and the code is written in a way that thoroughly denies the fact that GObject exists (which explains a lot of the excessive abstraction here). Of course, that would only help in the C marshaller case...
*** Bug 516776 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/494.