[vlc-devel] [RFC 0/2] New executor API

[Romain Vimont]

On Fri, Aug 28, 2020 at 11:16:21AM +0200, Romain Vimont wrote:
> On Fri, Aug 28, 2020 at 10:25:16AM +0200, Alexandre Janniaux wrote:
> > Hi,
> > 
> > On Thu, Aug 27, 2020 at 10:28:58PM +0300, Rémi Denis-Courmont wrote:
> > > Le torstaina 27. elokuuta 2020, 22.13.37 EEST Romain Vimont a écrit :
> > > > > So the runnable will be using poll(), vlc_cond_t, vlc_sem_t or some other
> > > > > wait method that supports timing-out. It might as well use that time-out
> > > > > capability instead of relying on a thread for the sole purpose of maybe
> > > > > (ab)using the interrupt callback on time-out.
> > > >
> > > > Yes, if you only want timeout, but not "unexpected" cancelation, that
> > > > works (and is better).
> > > >
> > > > > Besides, timing out is an error that deserves logging. Interrupt is not.
> > > > > So using the same callback for both purpose looks dubious to me.
> > > >
> > > > The goal is simplicity.
> > >
> > > By making things more complicated. Yeah right...
> > 
> > I agree with Remi, if you want simplicity, the best thing
> > would be to limit the asynchronous framework to the
> > asynchronous execution and signalling like almost every other
> > asynchronous framework.
> 
> For example, the Java concurrent framework provides a method cancel() on
> futures:
> https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/Future.html#cancel(boolean)
> 
> Of course, this is a higher level language, and thread interruption is
> something managed at the Thread level, so there is no custom interrupt
> callback to provide.
> 
> > Especially since youǘe talked about some `future` object, and
> > thus a pro-actor model which doesn't have ways to do
> > signalling from the outside of the asynchronous framework to
> > the task by itself in almost all implementation, and instead
> > have each layer of promise being able to return either a result
> > or an error that the future will catch, and an internal
> > composition rule for each layer's result.
> > 
> > In the case the vlc_executor is killed, the preparser client
> > needs to be killed first since it is using the executor, and
> > thus can cancel its tasks internally directly, which might be
> > at the middle of the preprocessing, at the beginning or right
> > before the end. It's much less convoluted than having the
> > preparser client use the vlc_executor to cancel the taks, which
> > will just really call the preparser specific cancellation code
> > anyway.
> 
> (The executor would still need a Cancel() function to remove the tasks
> not started yet.)
> 
> This would greatly simplify the executor itself.
> 
> But then, to get the same features, each user (like the preparser) will
> have to:
>  - still implement the interrupt mechanism for cancelation (it will just
>    not pass the callback to the executor)
>  - implement the timeout manually in addition (which came "for free" if
>    interruption was implemented)
>  - manage the list of its tasks submitted to the executor
>  - interrupt all remaining tasks before deleting the executor
> 
> Instead of just providing:
>  - the "interrupt" callback (that must be written for cancelation
>    anyway);
>  - the timeout parameter to vlc_executor_Submit().
> 
> In fact, there are 3 levels of features for the executor:
>  1. submit tasks (and cancel tasks not started yet)
>  2. cancel tasks with interruption (to be called on explicit cancel or
>     executor deletion)
>  3. timeout (calling the interrupt() callback)
> 
> Possibilities are basically either 1, 1+2 or 1+2+3.
> 
> IIUC, what you (Rémi & Alex) suggest is 1.
> 
> I don't really like the principle of an additional thread to trigger
> "interrupt" calls either, but I see that as a trade-off between
> simplicity for the executor impl VS simplicity for the users of the
> executor.
> 
> I initially implemented 1+2+3. The current version is:
> https://code.videolan.org/rom1v/vlc/commits/executor.16
> 
> I just added two commits to only get 1+2 (I removed the timeout):
> https://code.videolan.org/rom1v/vlc/commits/executor.17
> 
> As you can see, it greatly simplifies the executor:
> https://code.videolan.org/rom1v/vlc/-/commit/e25283cf4dbb03a0c145a4f4aea6da1ae9382126
> 
> But the users have to handle timeout manually (in addition to the
> interrupt mechanism):
> https://code.videolan.org/rom1v/vlc/-/commit/d838204802842e44fff23d7a6b46b69c338c617e

For completeness, I just added two more commits to get only (1.) (I also
removed the interrupt callback):
https://code.videolan.org/rom1v/vlc/commits/executor.18

The preparser has to call the interrupt on cancelation:
https://code.videolan.org/rom1v/vlc/-/commit/790fb59841a1cc40d0028702a6452ab5aea489b8

Regards