GNOME Bugzilla – Bug 744040
bin: Deadlock when sending event
Last modified: 2018-11-03 12:25:15 UTC
Encountered a locking order problem while doing some playing around in the playback test app: A flush occuring during a step leads to a basesink deadlock. The sink element is posting STEP_DONE, holding the STREAM_LOCK. The app sends a new step event from the sync bus handler, which tries to take the STATE_LOCK. Meanwhile, the app has sent a flushing seek, which is holding the STATE_LOCK, and trying to acquire the STREAM_LOCK. It seems either basesink should drop the stream lock while sending the STEP_DONE message, or the app should not send the new step event from the sync bus message handler, I'm not sure which.
+ Trace 234623
Thread 13 (Thread 0x7ff135534700 (LWP 30991))
(Maybe somehow be realated to bug 727949)
Created attachment 313771 [details] Test leading to deaklock Here's a test program, not exactly the same scenario but the same deadlock point.
Can also happen in the following scenario: 1) streaming thread of some element is waiting on a pad probe (stream lock) 2) gst_element_send_event() doing a SEEK, waiting for streaming thread to shut down (state lock -> stream lock) 3) gst_element_set_state() (state lock) 3) *would* unblock the pad, which would unblock the streaming thread and make everything happy... if there wasn't the state lock.
Created attachment 314331 [details] [review] element: Don't hold state lock all the time while sending an event This can cause various deadlocks, described in
Any reason why this is not a good idea? :)
It doesn't make gst-validate unhappy in any case.
Any comments, ideas, ...? Should we just do this and see what happens? :) Nobody seems to see a reason why the lock is there at all.
I'd guess the intent is to prevent the element from changing state while events are being processed - since state changes may cause unexpected deallocations and things. I think though, that anything that's sensitive to that sort of thing should already be protected with other mechanisms.
That might be the reason but protecting for that in this place seems wrong and it should also be protected elsewhere then anyway. Let's go ahead with this patch then?
Attachment 314331 [details] pushed as b427997 - element: Don't hold state lock all the time while sending an event
Let's give it a try then, we can still revert it if it is causing other problems. The commit message now contains further information commit b427997119a2b6aacbeb550f729936f8b963e24b Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Dec 10 11:35:05 2015 +0200 element: Don't hold state lock all the time while sending an event This lock seems to exist only to prevent elements from changing states while events are being processed. However events are going to be processed nonetheless in those elements if sent directly via pads, so protection must already be implemented inside the elements for event handling if it is needed. As such having the lock here is not very useful and is actually causing various deadlocks in different situations as described in https://bugzilla.gnome.org/show_bug.cgi?id=744040
commit a0b3a7f65878a844b1f983441749afdd3512ee54 Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Feb 17 16:41:02 2016 +0200 Revert "element: Don't hold state lock all the time while sending an event" This reverts commit b427997119a2b6aacbeb550f729936f8b963e24b. It breaks things that used to work before, even if the change by itself is correct and the previous code is just working around deeper bugs in the async state change code. Let's go back to what previously worked and then fix async state changes in general. https://bugzilla.gnome.org/show_bug.cgi?id=760532
-- 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/91.