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

[Alexandre Janniaux]

Hi,

On Wed, Sep 02, 2020 at 12:01:29PM +0200, Steve Lhomme wrote:
> 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.

IMHO, you're not supposed to delete the executor if you're still
using it. Doing otherwise would be a programming error in that case.

In particular, it provides a simpler design where you don't have to
handle «if the user might do this» scenario like yours.

> > > > 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.

Like written above, I see it the other way around. The executor is not
supposed to be deleted if you're still using it. The code actually not
do exactly that (it waits for the last task to be run) but still ensure
you're not adding new tasks and that all tasks have been cancelled or
achieved/being run. The fact it doesn't check for the last task and
wait for it is, IMHO, more an implementation detail that might make some
invalid code work but would assert on most invalid use cases, and trying
to have it fail on all invalid use case adds synchronization, thus adds
complexity that we don't want here.

If the scopes of the executors are including the scopes of the clients,
there's no way the executor would die before the tasks uses it. It would
not come to my mind to use a dangling pointer voluntarily knowing it can
be deleted somewhere else while I still need it, and this situation will
be catched by sanitizers.

Regards,
--
Alexandre Janniaux
Videolabs