GNOME Bugzilla – Bug 623491
make *_get_type() thread safe
Last modified: 2010-08-06 17:41:03 UTC
Created attachment 165193 [details] [review] patch to make _get_type() thread safe Some of these functions already take advantage of g_once_init_enter/leave() for thread safety, however, there are some left. I'm attaching a patch to fix this.
For the stuff in gst/*, there isn't really any need to use g_once_init_enter()/leave() there, because they're all ref'ed from within gst_init() already anyway. *Can* do it of course.. plugins/* we should fix I guess (could also just _ref() the type in the plugin_init or element's class_init though). Why do need to fix up the stuff in tests/check? It doesn't hurt of course, but why fix it if it ain't broken? And let's not touch stuff in tests/old/ (I very much doubt those even compile..)
well, I'm new to gstreamer, and not familiar with code of gstreamer. I just found that some _get_type() was thread safe, and some were not, and I remembered that they were always not thread safe before g_once_init_enter/leave was introduced to glib. So I assumed that this problem was caused by lacking of manpowers for the transmission, and I went ahead and tried to help. As I've said above, I didn't understand gstreamer code well, so I just checked the code already in gstreamer, as I've found g_once_init_enter/leave was already in gst/*, e.g. gst/gsttagsetter.c. For the tests/check, I think that depends on how you define "broken". For thread safe problems, it's really hard to prove it's broken. but I'd say it's as broken as any other code with static variables. Has anyone noticed a problem with the non-thread-safe _get_type()? Maybe the stuff in tests/old/ should be untouched, I just did a grep in the source tree and fix all _get_type()s. So if needed, I will split the patch.
Please split the patch, yes. Also I don't think this is necessary for plugin types (i.e. elements, indexers, typefinders) or enum/flag types used by them because they're initialized during plugin registration from the registry.
The splitted patches are attached.
Created attachment 166211 [details] [review] make-_get_type-in-gst-thread-safe.patch
Created attachment 166212 [details] [review] make-_get_type-in-plugins-thread-safe.patch
Created attachment 166214 [details] [review] make-_get_type-in-tests-check-thread-safe.patch
Created attachment 166215 [details] [review] make-_get_type-in-tests-old-thread-safe.patch
Comment on attachment 166215 [details] [review] make-_get_type-in-tests-old-thread-safe.patch This one is obsolete since the entire tests/old/ has been removed (also, this is 0.8 code and doesn't even compile).
Comment on attachment 166214 [details] [review] make-_get_type-in-tests-check-thread-safe.patch This I'll commit, but I'll avoid some of the type -> _type renames to keep the diff minimal. Hope there wasn't a reason for that. > For the tests/check, I think that depends on how you define "broken". For thread safe problems, it's really hard to prove it's broken. but I'd say it's as broken as any other code with static variables. Has anyone noticed a problem with the non-thread-safe _get_type()? These tests are for the most part all run in a single thread, that's why I said it wasn't strictly needed. Never seen or heard of a failure related to this on the build bots or elsewhere. But it doesn't cost us anything to fix it up I guess.
I committed the other two patches because using G_DEFINE_TYPE() prevents some copy&paste boilerplate code... and the two g_once_init_enter()/leave at the other places doesn't hurt and prevents people from using this as an bad example ;)
commit d5c0b3311b87381b0f741c142bee5c29f3605e70 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Aug 6 19:38:22 2010 +0200 bufferlist: Don't chain up finalize to the parent class GstMiniObject::finalize does nothing and this prevents a runtime-type-check cast and function call per buffer list. commit d41997040b0d17a579962ae6850e490cb8203503 Author: Shixin Zeng <zeng.shixin@gmail.com> Date: Tue Jul 20 09:23:11 2010 -0500 gst: make _get_type() in gst/* thread safe This is not really necessary here because everything is initialized from gst_init() already but using G_DEFINE_TYPE() removes some copy&paste boilerplate code. commit 98da78ed2a4d340d64724dbf671e42d5bc893ec7 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Aug 6 19:34:42 2010 +0200 plugins: Add declarations for _get_type() functions to fix compiler warnings commit cfefcc7183144c9e03f6051e5ba303c73aa33873 Author: Shixin Zeng <zeng.shixin@gmail.com> Date: Tue Jul 20 09:23:54 2010 -0500 plugins: Make *_get_type() in plugins/* thread safe It's not really needed here but using G_DEFINE_TYPE() reduces some copy&paste boilerplate code.