GNOME Bugzilla – Bug 675869
hlsdemux: potential dead-lock using GstTask
Last modified: 2014-02-12 15:07:10 UTC
Created attachment 213860 [details] [review] don't pause tasks when stopped patch The way that GstTask is used can potentially trigger a dead-lock situation when pausing a stopped task while another thread is trying to stop it. This can be resolved quite simply by not trying to pause the task when it is stopped, which doesn't make much sense anyway.
hlsdemux also tries to join without taking and releasing the task lock first, I suspect this is your real problem.
(In reply to comment #1) > hlsdemux also tries to join without taking and releasing the task lock first, I > suspect this is your real problem. I don't see how that would solve the problem since the other thread could potentially do anything between the release and the join. I suspect the real solution to this problem is to either disable state changes on GstTask during a join (with a suitable error), or make calls to set_state and join exclusive by adding a mutex to GstTask and lock it during join and set_state. The currently used object lock is not suitable since it is released in join on the GST_TASK_WAIT() call. Though the dead-lock would be solved, I still think my current patch makes sense to apply since it means that tasks won't be started just to be stopped again.
(In reply to comment #2) > (In reply to comment #1) > > hlsdemux also tries to join without taking and releasing the task lock first, I > > suspect this is your real problem. > > I don't see how that would solve the problem since the other thread could > potentially do anything between the release and the join. not if you wait for the task lock because then you are sure that the other thread is stopped and waiting and that you can do your join without any other thread interfering.
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > hlsdemux also tries to join without taking and releasing the task lock first, I > > > suspect this is your real problem. > > > > I don't see how that would solve the problem since the other thread could > > potentially do anything between the release and the join. > > not if you wait for the task lock because then you are sure that the other > thread is stopped and waiting and that you can do your join without any other > thread interfering. Sorry if I'm unclear, but I'm talking about a third thread which tries to change the state, not the task thread. Also, the join is documented to stop the task thread, so this should never be necessary.
(In reply to comment #4) > > Sorry if I'm unclear, but I'm talking about a third thread which tries to > change the state, not the task thread. State changes are protected with the state-lock on GstElement, only one state change can be done at a time. What kind of state change are you talking about that can be done from 2 threads? > > Also, the join is documented to stop the task thread, so this should never be > necessary. I agree that a concurrent pause and join should abort the pause operation. But you really want to avoid this situation, the pause call would fail and the called would probably not know what to do.
(In reply to comment #5) > (In reply to comment #4) > > Also, the join is documented to stop the task thread, so this should never be > > necessary. > > I agree that a concurrent pause and join should abort the pause operation. But > you really want to avoid this situation, the pause call would fail and the > called would probably not know what to do. This is exactly what this patch does. Any chance someone can apply it? I commented on the GstTask issue since there might be others modules that use it incorrectly and can potentially dead-lock.
commit 62ddd348071bc205529fd4b17e281d8772f0d200 Author: Gil Pedersen <git@gpost.dk> Date: Fri May 11 13:58:28 2012 +0200 hlsdemux: don't pause task when it is stopped This fixes a potential dead-lock situation from GstTask https://bugzilla.gnome.org/show_bug.cgi?id=675869
The code is still a bit scary around the GstTask usage but this should at least fix this potential problem.