[vlc-devel] [PATCH 1/2] interrupt: restore interrupted to the killed state

[Thomas Guillem]

On Thu, Oct 24, 2019, at 13:36, Rémi Denis-Courmont wrote:
> That's easy: drop the http alias from the old module (and rename it Icecast). 

We have the same issue with smb2 and libdsm.

> This bug has nothing to do with user abort; we shouldn't retry to connect to an unresponsive IP address.

"we shouldn't retry to connect to an unresponsive IP address." that is exactly what I'm trying to do.

> 
> Le 24 octobre 2019 13:47:02 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>> I have an other use case:
>> 
>>  1/ Open a http stream, server is not responding, the open function of the http2 module is waiting
>>  2/ The user cancel if after some time, the http2 open module is interrupted
>>  3/ The old http module is probed and will also wait indefinitely. The user can't cancel it anymore.
>> 
>> For me, it's clearly a bug, and a possible fix should be backported to VLC 3.0 too.
>> 
>> I agree that my 2 attempts to fix it are not good
>> 
>> 1/ Interrupt patch: rejected by you, I'll revert it
>> 2/ module probe patch: rejected by you. And indeed, breaking module loading even when the user cancel a media is not a perfect solution. For example it could prevent to load a module that will just save some contexts.
>> 
>> I will work on an other solution
>> 
>> On Thu, Oct 24, 2019, at 11:59, Rémi Denis-Courmont wrote:
>>> I don't agree with the problem statement. What it describes is intended behaviour, not a bug.
>>> 
>>> Le 24 octobre 2019 12:46:46 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>>>> 
>>>> On Thu, Oct 24, 2019, at 11:20, Rémi Denis-Courmont wrote:
>>>>> You effectively reverted to the old broken logic from before the interrupt system, and disregarding documented API semantics, wrecking many weeks of work from me. Is what you did.
>>>> 
>>>> Yes I made a mistake, I should have waited longer and/or ask again for review before pushing it.
>>>> I did not realize that my patch would cause such a mess. I'm sorry for that.
>>>> 
>>>> Is my alternate solution acceptable for you ?
>>>> 
>>>>> 
>>>>> Le 24 octobre 2019 10:02:17 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>>>>>> 
>>>>>> 
>>>>>> On Wed, Oct 23, 2019, at 21:25, Remi Denis-Courmont wrote:
>>>>>>> Le 2019-10-22 17:47, Thomas Guillem a écrit :
>>>>>>>> If a module is interrupted while waiting in a vlc_*_i11e*() function, 
>>>>>>>> the i11e
>>>>>>>> function will return an error after restoring the interrupted state 
>>>>>>>> (from
>>>>>>>> vlc_interrupt_finish()). This error will be carefully handled by the 
>>>>>>>> module
>>>>>>>> that will close itself. If a second module is in the probe list, any 
>>>>>>>> call to a
>>>>>>>> vlc_*_i11e*() function will act as not interrupted. This cause any 
>>>>>>>> following
>>>>>>>> modules in the probe list to ignore this interrupted state.
>>>>>>>> 
>>>>>>>> To fix this issue, vlc_interrupt_finish() will now restore the 
>>>>>>>> interrupted
>>>>>>>> state to the killed state. src/misc/interrupt.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/src/misc/interrupt.c b/src/misc/interrupt.c
>>>>>>>> index d2a04a72b3c..54810892fc1 100644
>>>>>>>> --- a/src/misc/interrupt.c
>>>>>>>> +++ b/src/misc/interrupt.c
>>>>>>>> @@ -151,7 +151,7 @@ static int vlc_interrupt_finish(vlc_interrupt_t 
>>>>>>>> *ctx)
>>>>>>>>      if (ctx->interrupted)
>>>>>>>>      {
>>>>>>>>          ret = EINTR;
>>>>>>>> -        ctx->interrupted = false;
>>>>>>>> +        ctx->interrupted = atomic_load(&ctx->killed);
>>>>>>> This looks wrong. The interruption is only there to signal the change in 
>>>>>>> status. It's not supposed to fire again if the status is not changed.
>>>>>> 
>>>>>> I sent this patch to open a discussion, I pushed the next day, maybe way too early, because vlc for iOS really need it in order to fix dialogs that are shown after the media is killed.
>>>>>> 
>>>>>> It's OK I can revert.
>>>>>> 
>>>>>> But I would like to discuss about the other way to fix the issue mentioned by this patch.
>>>>>> The only other fix I see if to abort the module probing (between 2 probes) if vlc_killed().
>>>>>> 
>>>>>> > 
>>>>>>>> }
>>>>>>>> vlc_mutex_unlock(&ctx->lock);
>>>>>>>> return ret;
>>>>>>> -- 
>>>>>>> Rémi Denis-Courmontvlc-devel mailing list
>>>>>>> To unsubscribe or modify your subscription options:
>>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>> vlc-devel mailing list
>>>>>> To unsubscribe or modify your subscription options:
>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>> 
>>>>> -- 
>>>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
>>>>> _______________________________________________
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>> 
>>> 
>>> -- 
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191024/bf18dc41/attachment.html>