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 635785 - basesrc: fix deadlock
basesrc: fix deadlock
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: High blocker
: 0.10.32
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-11-25 16:16 UTC by Håvard Graff (hgr)
Modified: 2011-01-10 12:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.80 KB, patch)
2010-11-25 16:16 UTC, Håvard Graff (hgr)
none Details | Review
proposed patch (1.17 KB, patch)
2011-01-10 12:21 UTC, Wim Taymans
none Details | Review

Description Håvard Graff (hgr) 2010-11-25 16:16:07 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)
Comment 1 Håvard Graff (hgr) 2010-11-25 16:25:32 UTC
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
Comment 2 David Schleef 2010-12-04 22:49:15 UTC
ping now that core is unfrozen
Comment 3 Wim Taymans 2010-12-29 10:51:08 UTC
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
Comment 4 Wim Taymans 2011-01-10 12:09:00 UTC
Proposed patch causes infinite loops in the base audio source.
Comment 5 Wim Taymans 2011-01-10 12:21:28 UTC
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.
Comment 6 Wim Taymans 2011-01-10 12:30:36 UTC
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