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 790468 - atomicqueue: not MT-safe and racy
atomicqueue: not MT-safe and racy
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-11-16 23:05 UTC by Matthew Waters (ystreet00)
Modified: 2018-11-03 12:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gst/atomicqueue: add some actual tests of functionality (4.37 KB, patch)
2017-11-16 23:06 UTC, Matthew Waters (ystreet00)
none Details | Review

Description Matthew Waters (ystreet00) 2017-11-16 23:05:38 UTC
_push() and _pop() from different threads can result in skips and/or memory corruption.
Comment 1 Matthew Waters (ystreet00) 2017-11-16 23:06:42 UTC
Created attachment 363869 [details] [review]
gst/atomicqueue: add some actual tests of functionality

A test the exhibits the behaviour.
Comment 2 Nicolas Dufresne (ndufresne) 2017-11-17 00:31:32 UTC
We should test with 1 pop threads and multiple push thread (the most common use through buffer pool). Will help figuring out how bad this is.
Comment 3 Sebastian Dröge (slomo) 2017-11-17 09:46:50 UTC
See GstBus' and GstBufferPool's usage of atomicqueue, you're supposed to add some kind of serialization of operations and waiting with GstPoll around it. There were also some commits for fixing issues around this in both files.

But maybe your problem is something else?
Comment 4 Matthew Waters (ystreet00) 2017-11-20 12:41:22 UTC
That seems to defeat the purpose of an atomic queue no if there's external synchronisation?  Especially if there's the potential for memory corruption (which there is).
Comment 5 Sebastian Dröge (slomo) 2017-11-20 12:46:43 UTC
Yes, but that's what Wim told me last time I looked into this (because of memory corruption in GstBus).

By having the synchronization outside the queue, you could potentially compose it better with other things that needs synchronization too. Like waiting for one out of many things instead of blocking on the queue directly.

Also the GstPoll is only atomic integer operations, unless it actually has to wait.
Comment 6 Matthew Waters (ystreet00) 2017-11-20 12:51:20 UTC
This is without any kind of synchronisation at all though.  e.g. replacing GAsyncQueue with GstAtomicQueue should not result in memory corruption.
Comment 7 Sebastian Dröge (slomo) 2017-11-20 13:02:37 UTC
It does because you need synchronization, otherwise you write things that you're just reading, etc.
Comment 8 Nicolas Dufresne (ndufresne) 2017-11-20 17:11:24 UTC
But we could also make it thread safe. If you have external synchronization it will just make it less often hit the spins, so "in theory" it shouldn't be slower. The documentation is currently completely miss-leading, because it's documented exactly the way Matthew started using it.
Comment 9 GStreamer system administrator 2018-11-03 12:43:04 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org'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.freedesktop.org/gstreamer/gstreamer/issues/256.