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

[Romain Vimont]

On Fri, Aug 28, 2020 at 07:11:55PM +0300, Rémi Denis-Courmont wrote:
> Le perjantaina 28. elokuuta 2020, 18.31.02 EEST Romain Vimont a écrit :
> > On Fri, Aug 28, 2020 at 04:51:35PM +0300, Rémi Denis-Courmont wrote:
> > > Le perjantaina 28. elokuuta 2020, 11.25.16 EEST Alexandre Janniaux a écrit 
> :
> > > > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > Yes exactly. All the complexity comes from the very disagreeable decision
> > > to have the runner rather than the owner keep track of the tasks. If the
> > > owner keeps track of its tasks, it can interrupt them (due to stop or
> > > time-out) directly, and the tasks can notify their complexition directly
> > > as well.
> > That's more or less what I quickly implemented on:
> > https://code.videolan.org/rom1v/vlc/commits/executor.18
> > https://mailman.videolan.org/pipermail/vlc-devel/2020-August/136784.html
> > 
> > (the on_finished() callback could also be removed)
> > 
> > The problem is that clients have to do a lot of (duplicate) work on
> > their own.
> 
> Code factorization is also the argument that the background worker had for 
> having all of its complexity. Like literally, see the archives (early 2017).
> 
> It seems rather self-contradictory to me that, on one hand, you blame 
> background_worker for complexity due to callbacks, and the other hand, you 
> propose inessential callbacks in the executor design.

I get your point.

It's the result of a trade-off: there is a gap between what a minimal
executor provides and what the actual users (preparser, etc.) of
executor need.

> > The executor must be able to queue runnables anyway (we want to be able
> > to queue 100 preparsing requests even if there are 4 threads).
> 
> You can argue all you want. Event delivery mechanisms (in a broad sense) 
> cannot be allowed to fail. It always ends up being used in places where errors 
> are impossible or prohibitively intricate to handle.
> 
> The only underlying stuff that can fail is memory allocation and thread 
> creation. If you make sure that there's always at least one runner thread, 
> there are no fatal errors except at runner allocation.
> 
> > So if the
> > caller just provides a runnable (a run callback) and a userdata pointer,
> > then the executor must be able to allocate something on _Submit() (so it
> > may fail) or limit the queue size (so it may also fail). Unless the
> > client also provides a structure with some data private to the executor
> > (e.g. a vlc_list node), but that looks ugly.
> > 
> > Moreover, if we don't want to spawn all the max_threads threads on
> > creation, we would like to spawn a thread as necessary on _Submit() (so
> > that may fail). IMO, it's not a problem that _Submit() returns an error
> > code.
> 
> Only one thread is needed to ensure that neither errors nor deadlocks ever 
> happen. If non-first threads fail to be created dynamically, you can always 
> fail safe to fewer threads than the maximum.

OK for thread creation, but what do you suggest to queue the submitted
runnables?