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

[Steve Lhomme]

On 2020-09-02 11:35, Alexandre Janniaux wrote:
> Hi,
> 
> On Wed, Sep 02, 2020 at 11:09:35AM +0200, Romain Vimont wrote:
>> On Wed, Sep 02, 2020 at 10:34:28AM +0200, Romain Vimont wrote:
>>> On Wed, Sep 02, 2020 at 08:13:15AM +0200, Steve Lhomme wrote:
>>>> On 2020-09-01 18:13, Romain Vimont wrote:
>>>>> +
>>>>> +void
>>>>> +vlc_executor_Submit(vlc_executor_t *executor, struct vlc_runnable *runnable)
>>>>> +{
>>>>> +    vlc_mutex_lock(&executor->lock);
>>>>> +
>>>>> +    assert(!executor->closing);
>>>>
>>>> It might be legitimate to try to submit a task even if the executor is
>>>> closing. For example if a runnable is trying to submit another runnable. It
>>>> should have to check with some higher power if it's allowed to do so or not
>>>> (atomically). So at least you should just return here.
>>>
>>> In fact, just returning here will still cause problems: once submitted,
>>> a runnable is either canceled (with vlc_executor_Cancel() returning
>>> VLC_SUCCESS) or run (the run() callback is called).
>>>
>>> This allows the caller to know if the task is "finished" (so that it can
>>> safely delete its resources).
>>>
>>> If a _Submit() is ignored, then the runnable will not be canceled and
>>> will not be run, so resources will leak.
>>>
>>> I will think about it. I would like not to return a bool from _Submit()

I said "return", not "return a value". I don't think a return value is 
needed. We do not have a "future" with the result of the task, otherwise 
it would go there.

>>> 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. And yet if it's not canceled 
it may submit a task when the executor just got canceled (by another 
thread). And you will end up in this assert.