GNOME Bugzilla – Bug 635785
basesrc: fix deadlock
Last modified: 2011-01-10 12:30:36 UTC
Created attachment 175245 [details] [review] patch Make sure to check the live_running variable after a LIVE_WAIT has been signaled. This could deadlock if wait_playing was called from outside basesrc. (baseaudiosrc)
More details: <hgr> baseaudiosrc... <hgr> setting it between PLAYING and PAUSED really quickly, lots of times... <hgr> you get the following chain of events: <hgr> 1. PAUSED -> gst_base_src_wait_playing() is waiting in GST_LIVE_WAIT <hgr> 2. PLAYING -> gst_base_src_set_playing(live_play=TRUE) and the LIVE_LOCK is taken <hgr> 3. the GST_LIVE_SIGNAL is signaled also inside gst_base_src_set_playing <hgr> 4. GST_LIVE_UNLOCK <hgr> Now the normal 5. would be that gst_base_src_wait_playing would continue...after set_playing had let go of the lock... <hgr> however, very race, now 5. is PAUSED-> gst_base_src_set_playing(live_play=FALSE) and the LIVE_LOCK is taken *before* the g_cond can take it again... <hgr> so then we get 6. GST_LIVE_UNLOCK inside gst_base_src_set_playing <hgr> and then <hgr> 7. gst_base_src_wait_playing continues <hgr> however, this ends up inside a 8. gst_ring_buffer_read () <wtay> oh <wtay> missing while around g_cond_wait() ? <hgr> and unless there is any buffer inside this read, it will block on GST_RING_BUFFER_WAIT() <hgr> and now the state-change goes to PLAYING again... <hgr> and then noone will signal GST_RINGBUFFER_SIGNAL (that only happends when going to PAUSED) <hgr> so you get a deadlock, where the state-changeing thread blocks on gst_base_src_set_playing(live_play=TRUE) on the LIVE_LOCK, that is held by the basesrc-thread, waiting for GST_RINGBUFFER_SIGNAL <hgr> the good news is that this only happens about every 1,5 million state-changes, the bad news is that it *can* happen, and hence should be fixed... :) <wtay> gst_base_src_wait_playing() is inside a loop that can only exit when live_running = TRUE <wtay> or maybe audiosrc does something special... <hgr> however, it would probably not fix this issue, because gst_base_src_set_playing needs to know that wait_playing has stopped waiting before it is safe to return <wtay> indeed <wtay> the audio base class doesn't have a while loop <wtay> bad <hgr> ah, yeah, I know what while-loop you meant now... the one inside gst_base_src... ! I am talking about audiobasesrc <wtay> it needs a while loop. either move it around in basesrc or add to audiosrc <wtay> just move the while loop in wait-playing <wtay> it actually makes the method do what its name says :) <hgr> I guess... so you would keep waiting if it is not yet running? <wtay> yes <wtay> like in the get_range function <hgr> exactly <hgr> I'll try it :) <wtay> great <hgr> it looks perfect! I have never been able to run for this long before... (*10) <wtay> yay <wtay> it makes sense, though <hgr> yup <hgr> I'll submit the patch asap <wtay> after a gcond, you always have to recheck the conditions that made you go into the gcond <wtay> ..always.. <hgr> I now have *100 better than my best run <hgr> I say no more deadlock <wtay> easy fix
ping now that core is unfrozen
slightly modified patch, check the condition before waiting. commit e444ffecf7568b6f9d23dae3dcef638674fe5661 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Wed Dec 29 11:48:18 2010 +0100 basesrc: fix deadlock Only go into LIVE_WAIT when the are not live_running and only stop waiting when live_running is TRUE. If we don't loop, we could deadlock when called from outside of basesrc, such as baseaudiosrc. Fixes #635785
Proposed patch causes infinite loops in the base audio source.
Created attachment 177925 [details] [review] proposed patch The original patch was more correct, we first need to release the lock and wait before doing anything else or else we have regressions in baseaudiosrc.
commit 5c479aa3a4f7a277b723a3d7608d80ee09bcff95 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Jan 10 13:18:16 2011 +0100 basesrc: make sure we wait and release the live lock Make sure we release the live lock and wait in all cases when we need to wait for the playing or flushing state change. Fixes #635785