[vlc-devel] [RFC v2 1/2] executor: introduce new executor API

[Alexandre Janniaux]

Hi,

On Wed, Sep 02, 2020 at 02:02:40PM +0200, Steve Lhomme wrote:
>
>
> On 2020-09-02 13:21, Romain Vimont wrote:
> > On Wed, Sep 02, 2020 at 01:00:49PM +0200, Steve Lhomme wrote:
> > > > > > > > just for that. I don't want to add a separate "deactivate" function for
> > > > > > > > closing before _Delete(). Maybe the threads should continue to dequeue
> > > > > > > > the pending tasks until the queue is empty in case a runnable queues
> > > > > > > > another one after "closing"?
> > > > > > >
> > > > > > > In fact, I think it is reasonable to assert that the executor is not
> > > > > > > closing. It is the caller responsibility to ensure that a runnable is
> > > > > > > not submitted after it called vlc_executor_Delete().
> > > > > >
> > > > > > I agree, you don't want to Delete the executor while you still have
> > > > > > pending tasks.
> > > > >
> > > > > That means any runnable that want to launch another runnable will need to
> > > > > check if the executor is not canceled.
> > > >
> > > > Yes. But this is the same "component" which provides the run() function
> > > > and which calls vlc_executor_Delete().
> > >
> > > I don't fully understand. But that doesn't sound right. That means the
> > > design is too tied to very narrow specific cases. (ie not chaining tasks)
> > >
> > > > > And yet if it's not canceled it may
> > > > > submit a task when the executor just got canceled (by another thread). And
> > > >
> > > > s/canceled/deleted/
> > > >
> > > > > you will end up in this assert.
> > > >
> > > > It is the responsibility of the caller not to submit tasks once
> > > > vlc_executor_Delete() is called.
> > >
> > > But there's a race condition, as explained (twice).
> >
> > Sorry, I don't see the race condition, assuming:
> >   - vlc_executor_Cancel() returns a bool indicating if the task has been
> >     removed from the pending queue, or on the contrary if it was already
> >     taken by an executor thread;
> >   - the caller is responsible, by any means (possibly setting a flag with
> >     proper synchronization before calling vlc_executor_Delete()), to not
> >     submit tasks once it called vlc_executor_Delete().
>
> Here's a sample code for what I described before:
>
> struct my_task {
>     vlc_executor_t *executor;
>     bool first;
>     char *str;
>     struct vlc_runnable runnable;
> };
>
> static void Run(void *userdata)
> {
>     struct my_task *task = userdata;
>
>     printf("start of %s\n", task->str);
>     vlc_tick_sleep(VLC_TICK_FROM_SEC(3)); // long action
>     printf("end of %s\n", task->str);
>
>     if (task->first)
>     {
>         struct my_task *newtask = malloc(sizeof(*newtask));
>         newtask->str = strdup(task->str);
>         newtask->first = false;
>         newtask->executor = task->executor;
>         newtask->runnable.run = Run;
>         newtask->runnable.userdata = newtask;
>
>         vlc_executor_Submit(task->executor, &newtask->runnable);
>     }
>
>     free(task->str);
>     free(task);
> }
>
> void foo(vlc_executor_t *executor, const char *str)
> {
>     // no error handling for brevity
>     struct my_task *task = malloc(sizeof(*task));
>     task->str = strdup(str);
>     task->first = true;
>     task->executor = executor;
>     task->runnable.run = Run;
>     task->runnable.userdata = task;
>     vlc_executor_Submit(executor, &task->runnable);
>
>     vlc_executor_Delete(executor);
> }
>
> If the first task is run it will try to run the second (within an executor
> thread) while the main thread has requested to delete all tasks.
>
> There is no way to do a Submit safely from an executor thread with the code
> you propose. It's possible it will assert.

First, I'm not sure I see the point of supporting this use
case. Here the fact there are two tasks is completely useless
since they are the only ones running, one after the other and
no parallel work can happen.

Then, since you'll need to wait for the tasks to complete
anyway, you can just execute the task directly in foo, removing
the need for the vlc_executor and the need to support this use
case at all.

I don't think it's reasonable to write the async framework
while having sync use cases and situation. Thus, concrete
examples might be a better fit for the design than minimal
codes which can't really enjoy an async design.

Here, one use case of the executor could be to have one task
scheduled on a poll() to check readiness of some file descriptors,
rescheduling itself as long as it's not interrupted (for instance
through a vlc_interrupt_t), and have it queue read or write tasks
to the given file descriptors.

First, this case is sandboxable so not impossible in the future.
It also describe the code through an async part and a sync part:

 - the poll and read/write tasks are async and potentially notifying
   the sync task of progress or status (error for example).

 - the «foo» task is sync and wait for the all the async tasks to
   finish, while setting up the interrupt framework if necessary.

What's important in an async design here is, IMHO, that if foo was
to quit before the tasks are finished, it would be an async operation
from the point of view of the caller of foo, and thus it cannot kill
its own state (vlc_executor) as long as the operation is not finished.

Regards,
--
Alexandre Janniaux
Videolabs