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

[Romain Vimont]

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.

Yes, I understood.

But returning silently means possible leaks (cf above). So one solution
to avoid these leaks could be returning a bool from _Submit() (but I
don't like it).

(but anyway, I prefer to keep this assert)

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

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

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