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 623491 - make *_get_type() thread safe
make *_get_type() thread safe
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal normal
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-07-03 20:53 UTC by Shixin Zeng
Modified: 2010-08-06 17:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to make _get_type() thread safe (17.25 KB, patch)
2010-07-03 20:53 UTC, Shixin Zeng
none Details | Review
make-_get_type-in-gst-thread-safe.patch (5.15 KB, patch)
2010-07-20 14:33 UTC, Shixin Zeng
committed Details | Review
make-_get_type-in-plugins-thread-safe.patch (6.04 KB, patch)
2010-07-20 14:34 UTC, Shixin Zeng
committed Details | Review
make-_get_type-in-tests-check-thread-safe.patch (3.76 KB, patch)
2010-07-20 14:35 UTC, Shixin Zeng
committed Details | Review
make-_get_type-in-tests-old-thread-safe.patch (3.11 KB, patch)
2010-07-20 14:35 UTC, Shixin Zeng
reviewed Details | Review

Description Shixin Zeng 2010-07-03 20:53:15 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.
Comment 1 Tim-Philipp Müller 2010-07-03 22:06:47 UTC
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..)
Comment 2 Shixin Zeng 2010-07-04 02:44:57 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2010-07-05 07:33:40 UTC
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.
Comment 4 Shixin Zeng 2010-07-20 14:32:35 UTC
The splitted patches are attached.
Comment 5 Shixin Zeng 2010-07-20 14:33:40 UTC
Created attachment 166211 [details] [review]
make-_get_type-in-gst-thread-safe.patch
Comment 6 Shixin Zeng 2010-07-20 14:34:28 UTC
Created attachment 166212 [details] [review]
make-_get_type-in-plugins-thread-safe.patch
Comment 7 Shixin Zeng 2010-07-20 14:35:20 UTC
Created attachment 166214 [details] [review]
make-_get_type-in-tests-check-thread-safe.patch
Comment 8 Shixin Zeng 2010-07-20 14:35:57 UTC
Created attachment 166215 [details] [review]
make-_get_type-in-tests-old-thread-safe.patch
Comment 9 Tim-Philipp Müller 2010-07-21 19:56:23 UTC
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 10 Tim-Philipp Müller 2010-07-21 20:41:35 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2010-08-06 17:40:39 UTC
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 ;)
Comment 12 Sebastian Dröge (slomo) 2010-08-06 17:41:03 UTC
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.