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

[Romain Vimont]

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.

Yes, this code is clearly wrong (the caller is responsible to never call
vlc_executor_Submit() once it called vlc_executor_Delete().

> There is no way to do a Submit safely from an executor thread with the code
> you propose. It's possible it will assert.

For example, you may:
 - add an "atomic_bool deleted" flag accessible from your "struct
   my_task"
 - set it before calling vlc_executor_Delete();
 - check it before calling vlc_executor_Submit() in Run().

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel