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

Thomas Guillem thomas at gllm.fr
Thu Oct 24 15:33:58 CEST 2019 

On Thu, Oct 24, 2019, at 15:27, Alexandre Janniaux wrote:
> 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?

We can't really, the interrupt API is very convenient for this kind of module. Such module can be stuck in dbus connection or when asking the passphrase to the user (using the OS/distrib keyring). If the media playback is stopped, these module need to be notified in order to abort the keystore connection immediately and cancel the passphrase question.

> 
> 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

More information about the vlc-devel mailing list