GNOME Bugzilla – Bug 64764
Class initialization isn't thread safe
Last modified: 2011-02-18 16:13:57 UTC
There is no protection against multiple threads trying to initialize a single object class at the same time in g_type_class_ref(). We probably need to do something like set an "initializing" flag on the type node and block other threads from using the class until that is cleared.
Putting on the 2.0.0 milestone for now.
Very unlikely to get to this for 2.0... more important things to worry about.
I don't understand this...what is the lock in g_type_class_ref() for then ? Is the problem that type_class_init_Wm() drops the lock in some places ?
the lock isn't and shouldn't be held across calls to app-provided callbacks like class_init. But we have to block other threads from using the type until the init functions have finished.
Created attachment 38490 [details] [review] untested patch
i'm sorry, but that patch is a joke, isn't it?
I certainly did not spend more than 10 minutes thinking about it. Unfortunately bugzilla ate the comment, where I said that I think that the init_state in the class structs already has the necessary information to block access to the class structs until they're fully initialized. The implementation of that idea may leave something to desire...
Created attachment 66276 [details] [review] Suggested patch Patch to lock the type node during class initialisation
Created attachment 66277 [details] Test program Program to test multi-threaded class initialisation
Note that even with the suggested patch, the programmer must still protect g_type_register_* and g_type_add_interface_* with GOnce (see Bug 65041 – _get_type() functions aren't thread safe).
Created attachment 66278 [details] [review] Amended suggested patch
ahunter, thanks for the patch, but i think it's too intrusive for what it does. i don't claim to have an exact idea of a solution at this point, but in any case, there shouldn't be more than involved than 1 new mutex+condition pair for all of gtype.c and maybe an extra bit or enum value per typenode. oh and, you don't need to wrap everything up in G_THREADS_ENABLED, that macro is mostly obsolete, we don't need to optimize-out the creation of new mutexes or conditions.
(In reply to comment #12) > ahunter, thanks for the patch, but i think it's too intrusive for what it does. Thanks for looking at it > i don't claim to have an exact idea of a solution at this point, but in any > case, there shouldn't be more than involved than 1 new mutex+condition pair for > all of gtype.c and maybe an extra bit or enum value per typenode. I decided that after g_type_class_ref returns, the class should really be fully initialised. That means waiting if another thread is in the process of initialising it. However some applications seem to ref the class inside the class initialisation. For example, gedit when invoking find/replace : the dialog uses a GtkCombo (deprecated) which uses a GtkList (deprecated) which ref's itself during initialisation). The result is that the thread ends up waiting for itself. To avoid that situation I made the flag hold the thread id of the thread using the node. That way you can detect recursion. By not sharing the mutex+condition, threads are only woken up when their own condition is met. > oh and, you don't need to wrap everything up in G_THREADS_ENABLED, that macro > is mostly obsolete, we don't need to optimize-out the creation of new mutexes > or conditions. > I added G_THREADS_ENABLED to allow the new members in TypeNode to be optimised -out. I notice now that the locking in the suggested patch is imperfect anyway.
*** Bug 451935 has been marked as a duplicate of this bug. ***
*** Bug 450722 has been marked as a duplicate of this bug. ***
i don't think the attached patch here gets all locking cases right (e.g. always acquires all mutexes in the same order to avoid deadlocks, and protects *all* user callbacks). the following should fix the issue however. 2008-02-05 18:52:07 Tim Janik <timj@imendio.com> * gtype.c: added recursive mutex to protect class initialization, default interface initialization and per-class interface construction. a lock to this recursive mutex is held during user callback invocations such as initializers or finalizers, effectively allowing only one thread to run class/interface initializers/finalizers at a time. also made misc fixups. this fixes: Bug 64764 - Class initialization isn't thread safe. also in SVN is a new test program (gobject/tests/threadtests.c) that tests concurrent class intializers, interface initializers and per-class interface constructors. it stress tests hard enough to reliably blow up without the above fix. (it doesn't test finalizers however, but i'd take patches to extend it in that direction ;)
This last fix prevents Geany to start, at least on Windows. I traced the problem to gtk_drawing_area_new which seems to "wait for itself". Since the implementation is simply: gtk_drawing_area_new (void) { return g_object_new (GTK_TYPE_DRAWING_AREA, NULL); } I don't know if it's a gtk or a glib bug.
(In reply to comment #17) > This last fix prevents Geany to start, at least on Windows. I traced the > problem to gtk_drawing_area_new which seems to "wait for itself". Since the > implementation is simply: > > gtk_drawing_area_new (void) > { > return g_object_new (GTK_TYPE_DRAWING_AREA, NULL); > } > > I don't know if it's a gtk or a glib bug. there is no problem with gtk_drawing_area_new() calling g_object_new(GTK_TYPE_DRAWING_AREA), since drawing area initialization will not call gtk_drawing_area_new(). having a _new() function around instance creation with g_object_new() is a well established practice with many widgets.
> there is no problem with gtk_drawing_area_new() calling > g_object_new(GTK_TYPE_DRAWING_AREA), since drawing area initialization will not > call gtk_drawing_area_new(). having a _new() function around instance creation > with g_object_new() is a well established practice with many widgets. > I supposed so. Just to make sure, I filed a bug at Geany too. Since exactly the upgrade from glib 2.15.3 to 2.15.4 broke things (this patch), I thought that maybe you have an idea of what could happen. From my point of view, this can be related to one of glib, gtk, geany or gtk-scintilla or even a couple of them. Thanks.
*** Bug 515802 has been marked as a duplicate of this bug. ***
Similarly starting ~many Gnome apps I get: (gnome-screenshot:7966): GLib-GObject-WARNING **: IA__g_object_new_valist: object class `GThemedIcon' has no property named `names' or similar; the ultimate cause of which is the same - racing class instantiation vs. class init.
bug 537555 is interestingly related :-)
(In reply to comment #21) > Similarly starting ~many Gnome apps I get: > > (gnome-screenshot:7966): GLib-GObject-WARNING **: IA__g_object_new_valist: > object class `GThemedIcon' has no property named `names' > > or similar; the ultimate cause of which is the same - racing class > instantiation vs. class init. This is way too little information to be useful. It is possible however that you're seeing a variant of Bug #522247.
sure, that's certainly the case :-)