<p>Hi Thomas,</p>
<h3 id="race-when-an-entity-is-starting-to-be-preparsed">Race when an entity is starting to be preparsed</h3>
<pre><code>/* src/playlist/preparser.c */
252 var_AddCallback( input, "intf-event", InputEvent, preparser );
253 if( input_Start( input ) == VLC_SUCCESS )
254 {
255 vlc_mutex_lock( &preparser->lock );
256
257 preparser->input_state = INPUT_RUNNING;
258 preparser->input_id = p_entry->id;</code></pre>
<p>There is a race between the call to <code>input_Start</code> (<code>#1</code>) and later setting the state to <code>INPUT_RUNNING</code> (<code>#2</code>).</p>
<p>If the <code>"intf-event"</code> callback is trigger in-between <code>#1</code> and <code>#2</code>, the signal from the callback is lost, but more importantly the input state is still <code>INPUT_INIT</code> meaning that the callback does not update the state to <code>INPUT_STOPPED</code> when the input is finished.</p>
<p>If the above happens, and:</p>
<ul>
<li><p>if we have a <em>preparse-timeout</em>: <code>input_item_SignalPreparsedEnded</code> will send <code>ITEM_PREPARSE_TIMEOUT</code> after the timeout has passed, even though the the <em>input-thread</em> stopped on its own.</p></li>
<li><p>unless we have a <em>preparse-timeout</em>: The preparser is stuck waiting for a signal/change-of-value that will never come (preventing other entities from being preparsed).</p></li>
</ul>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="race-regarding-playlist_preparser_cancel">Race regarding <code>playlist_preparser_Cancel</code></h3>
<p>This is similar to the previously described race, though it will happen if a cancellation for entity <code>X</code> is requested after the relevant <code>p_entry</code> has been removed from <code>p_preparser->pp_waiting</code>, but before <code>p_preparser->input_id</code> has been set to refer to the item.</p>
<p>If this is expected behavior (that we will still run an entity even though it has been requested to be canceled), <code>playlist_preparser_Cancel</code> should probably be renamed to something that reflects the purpose, such as; <code>playlist_preparser_CancelRequest</code>.</p>
<p>We do, however, need a true <code>playlist_preparser_Cancel</code> in order to fix the races between a running preparser, and an item that is scheduled to be played (in the playlist).</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<p>On 2016-09-27 19:11, Thomas Guillem wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> vlc | branch: master | Thomas Guillem <thomas@gllm.fr> | Tue Sep 27
15:58:53 2016 +0200| [94def8559b5e45cfb58d2d0d477d86a8dab17a98] |
committer: Thomas Guillem
preparser: fix timeout
input_Stop() must be called before input_Close() in case of timeout.
closes #17353</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<h3 id="httpgit.videolan.orggitweb.cgivlc.gitacommith94def8559b5e45cfb58d2d0d477d86a8dab17a98">http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=94def8559b5e45cfb58d2d0d477d86a8dab17a98</h3>
</blockquote>
<pre><code> src/playlist/preparser.c | 56 ++++++++++++++++++++++++++++++------------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/src/playlist/preparser.c b/src/playlist/preparser.c
index 0dc31e5..e4a219c 100644
--- a/src/playlist/preparser.c
+++ b/src/playlist/preparser.c
@@ -52,9 +52,13 @@ struct playlist_preparser_t
playlist_fetcher_t *p_fetcher;
mtime_t default_timeout;
- input_thread_t *input;
void *input_id;
- bool input_done;
+ enum {
+ INPUT_INIT,
+ INPUT_RUNNING,
+ INPUT_STOPPED,
+ INPUT_CANCELED,
+ } input_state;
vlc_mutex_t lock;
vlc_cond_t wait;
@@ -75,9 +79,8 @@ playlist_preparser_t *playlist_preparser_New( vlc_object_t *parent )
if( !p_preparser )
return NULL;
- p_preparser->input = NULL;
p_preparser->input_id = NULL;
- p_preparser->input_done = false;
+ p_preparser->input_state = INPUT_INIT;
p_preparser->object = parent;
p_preparser->default_timeout = var_InheritInteger( parent, "preparse-timeout" );
p_preparser->p_fetcher = playlist_fetcher_New( parent );
@@ -149,8 +152,8 @@ void playlist_preparser_Cancel( playlist_preparser_t *p_preparser, void *id )
/* Stop the input_thread reading the item (if any) */
if( p_preparser->input_id == id )
{
- assert( p_preparser->input != NULL );
- input_Stop( p_preparser->input );
+ p_preparser->input_state = INPUT_CANCELED;
+ vlc_cond_signal( &p_preparser->thread_wait );
}
vlc_mutex_unlock( &p_preparser->lock );
}
@@ -167,8 +170,8 @@ void playlist_preparser_Delete( playlist_preparser_t *p_preparser )
REMOVE_ELEM( p_preparser->pp_waiting, p_preparser->i_waiting, 0 );
}
- if( p_preparser->input != NULL )
- input_Stop( p_preparser->input );
+ p_preparser->input_state = INPUT_CANCELED;
+ vlc_cond_signal( &p_preparser->thread_wait );
while( p_preparser->b_live )
vlc_cond_wait( &p_preparser->wait, &p_preparser->lock );
@@ -196,8 +199,15 @@ static int InputEvent( vlc_object_t *obj, const char *varname,
if( event == INPUT_EVENT_DEAD )
{
- preparser->input_done = true;
- vlc_cond_signal( &preparser->thread_wait );
+ vlc_mutex_lock( &preparser->lock );
+
+ if( preparser->input_state != INPUT_INIT )
+ {
+ preparser->input_state = INPUT_STOPPED;
+ vlc_cond_signal( &preparser->thread_wait );
+ }
+
+ vlc_mutex_unlock( &preparser->lock );
}
(void) obj; (void) varname; (void) old;
@@ -244,26 +254,29 @@ static void Preparse( playlist_preparser_t *preparser,
{
vlc_mutex_lock( &preparser->lock );
- preparser->input = input;
- preparser->input_done = false;
+ preparser->input_state = INPUT_RUNNING;
preparser->input_id = p_entry->id;
if( p_entry->timeout > 0 )
{
mtime_t deadline = mdate() + p_entry->timeout;
- int ret = 0;
- while( !preparser->input_done && ret == 0 )
- ret = vlc_cond_timedwait( &preparser->thread_wait,
- &preparser->lock, deadline );
- status = ret == 0 ? ITEM_PREPARSE_DONE : ITEM_PREPARSE_TIMEOUT;
+ while( preparser->input_state == INPUT_RUNNING )
+ {
+ if( vlc_cond_timedwait( &preparser->thread_wait,
+ &preparser->lock, deadline ) )
+ preparser->input_state = INPUT_CANCELED; /* timeout */
+ }
}
else
{
- while( !preparser->input_done )
+ while( preparser->input_state == INPUT_RUNNING )
vlc_cond_wait( &preparser->thread_wait, &preparser->lock );
- status = ITEM_PREPARSE_DONE;
}
- preparser->input = NULL;
+ assert( preparser->input_state == INPUT_STOPPED
+ || preparser->input_state == INPUT_CANCELED );
+ status = preparser->input_state == INPUT_STOPPED ?
+ ITEM_PREPARSE_DONE : ITEM_PREPARSE_TIMEOUT;
+ preparser->input_state = INPUT_INIT;
preparser->input_id = NULL;
vlc_mutex_unlock( &preparser->lock );
@@ -272,6 +285,8 @@ static void Preparse( playlist_preparser_t *preparser,
status = ITEM_PREPARSE_FAILED;
var_DelCallback( input, "intf-event", InputEvent, preparser );
+ if( status == ITEM_PREPARSE_TIMEOUT )
+ input_Stop( input );
input_Close( input );
var_SetAddress( preparser->object, "item-change", p_item );
@@ -280,7 +295,6 @@ static void Preparse( playlist_preparser_t *preparser,
}
else if (!b_preparse)
input_item_SignalPreparseEnded( p_item, ITEM_PREPARSE_SKIPPED );
-
}
/**
_______________________________________________
vlc-commits mailing list
vlc-commits@videolan.org
https://mailman.videolan.org/listinfo/vlc-commits</code></pre>
</blockquote>