[vlc-devel] [vlc-commits] preparser: fix timeout
Filip Roséen
filip at atch.se
Tue Sep 27 22:01:17 CEST 2016
Hi Thomas,
Race when an entity is starting to be preparsed
-----------------------------------------------
/* 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;
There is a race between the call to `input_Start` (`#1`) and later
setting the state to `INPUT_RUNNING` (`#2`).
If the `"intf-event"` callback is trigger in-between `#1` and `#2`,
the signal from the callback is lost, but more importantly the input
state is still `INPUT_INIT` meaning that the callback does not update
the state to `INPUT_STOPPED` when the input is finished.
If the above happens, and:
- if we have a *preparse-timeout*: `input_item_SignalPreparsedEnded`
will send `ITEM_PREPARSE_TIMEOUT` after the timeout has passed,
even though the the *input-thread* stopped on its own.
- unless we have a *preparse-timeout*: The preparser is stuck
waiting for a signal/change-of-value that will never come
(preventing other entities from being preparsed).
---------------------------------------------------------------------
Race regarding `playlist_preparser_Cancel`
---------------------------------------------------------------------
This is similar to the previously described race, though it will
happen if a cancellation for entity `X` is requested after the
relevant `p_entry` has been removed from `p_preparser->pp_waiting`,
but before `p_preparser->input_id` has been set to refer to the item.
If this is expected behavior (that we will still run an entity even
though it has been requested to be canceled), `playlist_preparser_Cancel`
should probably be renamed to something that reflects the purpose,
such as; `playlist_preparser_CancelRequest`.
We do, however, need a true `playlist_preparser_Cancel` in order to
fix the races between a running preparser, and an item that is
scheduled to be played (in the playlist).
---------------------------------------------------------------------
---------------------------------------------------------------------
On 2016-09-27 19:11, Thomas Guillem wrote:
> vlc | branch: master | Thomas Guillem <thomas at 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
>
> > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=94def8559b5e45cfb58d2d0d477d86a8dab17a98
> ---
>
> 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 at videolan.org
> https://mailman.videolan.org/listinfo/vlc-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20160927/b4ba5001/attachment.html>
More information about the vlc-devel
mailing list