GNOME Bugzilla – Bug 540256
[patch] g_signal_connect_type and friends
Last modified: 2008-06-26 14:18:57 UTC
Please describe the problem: This adds a simple wrapper around g_signal_add_emission_hook, where the connect/callback signatures will be exactly like those for g_signal_connect. For example, g_signal_connect_type(GTK_TYPE_WIDGET, "show", G_CALLBACK(widget_show_cb), data); I have tried to follow the standard coding style in the patch, and have added some documentation blurb. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 113446 [details] [review] patch
Thanks, a few words on the patch... It would have been easier to create the closure in g_signal_connect_type_flags, get rid of g_signal_connect_closure_by_id in signal_emission_hook_fn and always just call g_closure_invoke there. However, I'm generally opposed to extend the current emission hook functionality or even make it more convenient. The reason for this is that it's allmost always a bad idea to use emission hooks at all, they are very inefficient to use because they impact the performance of *all* signal emissions, and they are very bug prone because doing much more than setting a simple flag in an emission hook (similar to constraints of a unix signal handler) is likely to trigger hard-to-debug reentrancy problems, because not all objects implementations are fully reentrant for every possible signal emission. The very few cases that are valid uses for signal emission hooks are automatica GUI sound generation (libcanberra) and a few accessibility cases, and those work perfectly fine with the current emission hook signature.
We plan on using this in libpurple, which is single-threaded. Hopefully we won't experience the possible reentrancy problems. I will update our code according to your suggestion. Thanks for the quick review! :)
(In reply to comment #3) > Hopefully we won't experience the possible reentrancy problems. You probably will some day. I'm not talking about thread-reentrancy here, but simply reentrancy into object methods during signal emissions or callback execution. Consider the following simplistic scenario: /* invariant for this getter: o->string is always a valid pointer */ char* get_string (Object *o) { return o->string; } accessor (Object *o, char *string) { /* object state is consistent and meets all invariants */ g_free (o->string); /* o->string contains a dangling pointer, invariants broken */ signal_emit (o, "foo"); /* o->string contains a dangling pointer, invariants broken */ o->string = g_strdup (string); /* object state is consistent and meets all invariants */ } Anyone connecting to ::foo should rather not call get_string(). Maybe this can be overseen for all signal handlers connected to ::foo on the object, but most probably not for general purpose emission hooks. While this example can be easily fixed with assigning o->string before the signal emission, many real world cases cannot. Particularly not when signal emissions are used to notify or implement object state transitions, like e.g. GtkWidget::realize.