[vlc-devel] [PATCH] Avoid deadlock on stop() using UDP on Android

[Romain Vimont | ®om]

My explanation about pthread_cancel() is inexact: vlc_cancel() is called
in udp.c/Close(), so it is causally dependent of the cond_wait()
wake up. Therefore, this is wrong to say that vlc_cancel() should wake
up cond_wait().

The lack of pthread_cancel() only prevent to fix the problem calling
vlc_cancel() earlier. So there is no reason the deadlock can not be
reproduced on other platforms, that this patch should fix.

Le vendredi  9 mai 2014 à 17:58 +0200, Romain Vimont a écrit :
> Android has no pthread_cancel(). Its behavior is emulated in
> vlc_cancel() using a flag "killed", but it can lead to unavoidable race
> conditions, typically between a call to vlc_object_alive() and a
> blocking cond_wait().
> 
> This leads to reproducible (with an acceptable probability) deadlocks
> when stopping just after playing UDP streams on Android:
> https://mailman.videolan.org/pipermail/vlc-devel/2014-April/097861.html
> https://mailman.videolan.org/pipermail/vlc-devel/2014-April/097912.html
> 
> Thus, we need to be able to wake up cond_wait() once the thread has to
> finish. The commit 57eee64 did it, but it fails when block_FifoWake() is
> called while the queue is not empty (in that case, it has no effect).
> This is probably not a problem on platforms where pthread_cancel() is
> available (the cond_wait() would be awakened), but this is a problem on
> Android.
> 
> It seems the "b_force_wake" flag semantic could not be modified to match
> a "finishing" state (e.g. block_FifoWake() is called in
> input_DecoderWait()). Therefore, a separate flag "b_finishing" is added,
> and set at the end of the UDP ThreadRead(), leading to set the "b_eof"
> flag in the access_t.info structure.
> 
> With these changes, I cannot reproduce the deadlock at all.
> 
> Signed-off-by: Romain Vimont <rom at rom1v.com>
> ---
>  include/vlc_block.h  |    2 ++
>  modules/access/udp.c |   10 ++++++++--
>  src/misc/block.c     |   39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/include/vlc_block.h b/include/vlc_block.h
> index 75f98ff..264fd95 100644
> --- a/include/vlc_block.h
> +++ b/include/vlc_block.h
> @@ -313,6 +313,8 @@ VLC_API void block_FifoPace( block_fifo_t *fifo, size_t max_depth, size_t max_si
>  VLC_API void block_FifoEmpty( block_fifo_t * );
>  VLC_API size_t block_FifoPut( block_fifo_t *, block_t * );
>  VLC_API void block_FifoWake( block_fifo_t * );
> +VLC_API void block_FifoFinish( block_fifo_t * );
> +VLC_API bool block_FifoIsFinishing( block_fifo_t * );
>  VLC_API block_t * block_FifoGet( block_fifo_t * ) VLC_USED;
>  VLC_API block_t * block_FifoShow( block_fifo_t * );
>  size_t block_FifoSize( const block_fifo_t *p_fifo ) VLC_USED;
> diff --git a/modules/access/udp.c b/modules/access/udp.c
> index 3c18cb8..29d5c53 100644
> --- a/modules/access/udp.c
> +++ b/modules/access/udp.c
> @@ -231,7 +231,13 @@ static block_t *BlockUDP( access_t *p_access )
>  {
>      access_sys_t *sys = p_access->p_sys;
>  
> -    return block_FifoGet( sys->fifo );
> +    block_t *block = block_FifoGet( sys->fifo );
> +    if( unlikely( block_FifoIsFinishing( sys->fifo ) ) )
> +    {
> +        /* We should not read data anymore */
> +        p_access->info.b_eof = true;
> +    }
> +    return block;
>  }
>  
>  /*****************************************************************************
> @@ -270,6 +276,6 @@ static void* ThreadRead( void *data )
>          block_FifoPut( sys->fifo, pkt );
>      }
>  
> -    block_FifoWake( sys->fifo );
> +    block_FifoFinish( sys->fifo );
>      return NULL;
>  }
> diff --git a/src/misc/block.c b/src/misc/block.c
> index 3e953f1..fdfb89c 100644
> --- a/src/misc/block.c
> +++ b/src/misc/block.c
> @@ -520,6 +520,7 @@ struct block_fifo_t
>      size_t              i_depth;
>      size_t              i_size;
>      bool          b_force_wake;
> +    bool                b_finishing;
>  };
>  
>  block_fifo_t *block_FifoNew( void )
> @@ -535,6 +536,7 @@ block_fifo_t *block_FifoNew( void )
>      p_fifo->pp_last = &p_fifo->p_first;
>      p_fifo->i_depth = p_fifo->i_size = 0;
>      p_fifo->b_force_wake = false;
> +    p_fifo->b_finishing = false;
>  
>      return p_fifo;
>  }
> @@ -647,6 +649,35 @@ void block_FifoWake( block_fifo_t *p_fifo )
>  }
>  
>  /**
> + * Set the "finishing" flag, and force the queue to wake up.
> + * This flag is intented to avoid deadlocks.
> + *
> + * @param p_fifo the queue
> + */
> +void block_FifoFinish( block_fifo_t *p_fifo )
> +{
> +    vlc_mutex_lock( &p_fifo->lock );
> +    p_fifo->b_force_wake = true;
> +    p_fifo->b_finishing = true;
> +    vlc_cond_broadcast( &p_fifo->wait );
> +    vlc_mutex_unlock( &p_fifo->lock );
> +}
> +
> +/**
> + * Indicate whether the fifo is in "finishing" state.
> + *
> + * @param p_fifo the queue
> + */
> +bool block_FifoIsFinishing( block_fifo_t *p_fifo )
> +{
> +    bool res;
> +    vlc_mutex_lock( &p_fifo->lock );
> +    res = p_fifo->b_finishing;
> +    vlc_mutex_unlock( &p_fifo->lock );
> +    return res;
> +}
> +
> +/**
>   * Dequeue the first block from the FIFO. If necessary, wait until there is
>   * one block in the queue. This function is (always) cancellation point.
>   *
> @@ -667,6 +698,14 @@ block_t *block_FifoGet( block_fifo_t *p_fifo )
>          vlc_cond_wait( &p_fifo->wait, &p_fifo->lock );
>  
>      vlc_cleanup_pop();
> +
> +    if( p_fifo->b_finishing )
> +    {
> +        /* The fifo is finishing, we should not handle new blocks */
> +        vlc_mutex_unlock( &p_fifo->lock );
> +        return NULL;
> +    }
> +
>      b = p_fifo->p_first;
>  
>      p_fifo->b_force_wake = false;
> -- 
> 1.7.10.4
>