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

Rémi Denis-Courmont remi at remlab.net
Thu Oct 24 17:09:43 CEST 2019 

The interrupt API is only meant for crossing layers polling I/O, namely demuxers blocking on stream filter/access. Because of the fatally blocking design of the demuxer API. For anything newer than the interrupt system, it's best to design the API properly to begin with.

So yeah, using it for anything else is not typically such a great idea.

Le 24 octobre 2019 16:27:10 GMT+03:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
>Hi,
>
>That's a good point for keystore modules. Is there a
>solution or should we just stop using the interrupt API for
>these use cases?
>
>Regards,
>--
>Alexandre Janniaux
>Videolabs
>
>On Thu, Oct 24, 2019 at 03:12:30PM +0200, Thomas Guillem wrote:
>> There is the same problem with keystore modules. 2 (secret and
>kwallet) are using the i11e API. If you abort the first one, the second
>one will wait.
>>
>> There is likely the same issue with every modules that use the i11e
>API from their probe function.
>>
>> On Thu, Oct 24, 2019, at 14:14, Thomas Guillem wrote:
>> >
>> > On Thu, Oct 24, 2019, at 13:52, Rémi Denis-Courmont wrote:
>> >> Hi,
>> >>
>> >> It is the same bug to the extent that:
>> >> - there should not be more than one access module per network URI
>scheme, and
>> > Why ? There is no documentation about that.
>> >
>> >> - it does not relate to the interrupt framework.
>> >>
>> >> It is not the same to the extent that the sad HTTP situation
>exists under the grand father clause. I already cut the number of HTTP
>handlers from original three (HTTP 1.1, HTTP 1.0, MMSH) to two (new and
>old), and just now suggested how to get to just one. SMB never had more
>than one handler in any release.
>> >
>> > libsmb2 is merged in VLC 3.0 now. Android and iOS ports will need
>these 2 modules for a long time.
>> >
>> >>
>> >> Le 24 octobre 2019 14:39:14 GMT+03:00, Thomas Guillem
><thomas at gllm.fr> a écrit :
>> >>>
>> >>> 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
>> >>>
>> >>
>> >> --
>> >> 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
>> >
>> > _______________________________________________
>> > vlc-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
>_______________________________________________
>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é.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191024/33b00cd5/attachment.html>

More information about the vlc-devel mailing list