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

[Steve Lhomme]

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.