[vlc-devel] [PATCH] HLS: remove some useless mutex locks

[Frederic YHUEL]

Hugo, thanks for your explanation!

See my inline comments/questions. Or don't. I will sleep on it and
maybe save you some time :-)

On Tue, Feb 7, 2012 at 4:07 PM, Jean-Paul Saman <jpsaman at videolan.org> wrote:
> On Tue, Feb 7, 2012 at 3:26 PM, Hugo Beauzée-Luyssen <beauze.h at gmail.com> wrote:
>> 2012/2/7 Frédéric Yhuel <fyhuel at viotech.net>:
>>> ---
>>>  modules/stream_filter/httplive.c |    4 ----
>>>  1 files changed, 0 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/modules/stream_filter/httplive.c b/modules/stream_filter/httplive.c
>>> index c03061b..9afd5c0 100644
>>> --- a/modules/stream_filter/httplive.c
>>> +++ b/modules/stream_filter/httplive.c
>>> @@ -2070,9 +2070,7 @@ static void Close(vlc_object_t *p_this)
>>>     assert(p_sys->hls_stream);
>>>
>>>     /* */
>>> -    vlc_mutex_lock(&p_sys->download.lock_wait);
>>>     vlc_cond_signal(&p_sys->download.wait);
>>> -    vlc_mutex_unlock(&p_sys->download.lock_wait);
>>>
>>
>> This mutex isn't useless, if you don't hold it before calling
>> vlc_cond_signal there is no way of being sure the thread is actually
>> waiting.
>> If the mutex is held by the download thread, it's likely to enter the
>> wait condition, thus the call to vlc_thread_join would stall.
>> Moreover, I think there are some slight chances of deadlock when
>> calling Close(), and some non thread-safe access to variables such as
>> as p_sys.(download|playback).segment...
>> Anyway, I'm pretty sure this mutex is not useless.
>>

I understand the problem but I don't really understand why this lock
is a good solution.

I think it's possible to acquire the lock just before the download
thread does, and then we have the exact same problem.

>>>     /* */
>>>     if (p_sys->b_live)
>>> @@ -2223,9 +2221,7 @@ static ssize_t hls_Read(stream_t *s, uint8_t *p_read, unsigned int i_read)
>>>             vlc_mutex_unlock(&segment->lock);
>>>
>>>             /* signal download thread */
>>> -            vlc_mutex_lock(&p_sys->download.lock_wait);
>>>             vlc_cond_signal(&p_sys->download.wait);
>>> -            vlc_mutex_unlock(&p_sys->download.lock_wait);
>>
>> The same applies here, if the hls_Thread is about to enter
>> vlc_cond_wait (ie. having the download.lock held) if you don't acquire
>> the mutex first in the main thread, you might signal the thread before
>> it enters the vlc_cond_wait, thus leading to unexpected behavior.
>>

I thought the signal would be lost, without causing any damages. Wouldn't it?


>
> Hugo,
>
> You are correct.
>
> Kind regards
>
> Jean-Paul Saman

Jean-Paul, thanks for the confirmation.

Best Regards,

-- 
Frédéric