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

[Romain Vimont]

On Wed, Aug 26, 2020 at 10:01:19PM +0300, Rémi Denis-Courmont wrote:
> Le keskiviikkona 26. elokuuta 2020, 20.19.56 EEST Romain Vimont a écrit :
> > +/**
> > + * Create a new executor.
> > + *
> > + * \param parent      a VLC object
> > + * \param max_threads the maximum number of threads used to execute
> > runnables + * \return a pointer to a new executor, or NULL if an error
> > occurred + */
> > +VLC_API vlc_executor_t *
> > +vlc_executor_New(vlc_object_t *parent, unsigned max_threads);
> 
> Don't really get why a VLC object is needed here. This can be limiting for 
> what's supposed to be a generic helper.

True. Will remove.

> > + * \code{c}
> > + *  static void Run(void *userdata)
> > + *  {
> > + *      char *str = userdata;
> > + *      printf("start of %s\n", str);
> > + *      sleep(3);
> 
> ONCE AGAIN, sleep is not portably thread-safe. This is not a good example.

Because it may be implemented using alarm?

I could replace by vlc_tick_sleep().

> 
> > + *      printf("end of %s\n", str);
> > + *  }
> 
> > + *  void foo(vlc_executor_t *executor, const char *name)
> > + *  {
> > + *      // error handling replaced by assertions for brevity
> > + *      char *str = strdup(name);
> > + *      assert(str);
> 
> Example code should not assume !defined(NDEBUG) either.

That's just an example to illustrate, I could even remove the assert()
and ignore error handling.

> 
> > + *      int ret = vlc_executor_Submit(executor, &runnable, str, NULL, 0);
> > + *      assert(ret == VLC_SUCCESS);
> > + *  }
> > + * \endcode
> 
> > +VLC_API int
> > +vlc_executor_Submit(vlc_executor_t *executor,
> > +                    const struct vlc_runnable *runnable, void *userdata,
> > +                    void *id, vlc_tick_t timeout);
> 
> Type name for works and workers really ought to be in the same semantic field 
> IMO.

(the naming was inpired from ExecutorService in Java, but Executor is
quite "standard" in other languages too.)
> 
> It is unlikely that callers will (or even should have to) handle errors well 
> at this point.

The error handling is simple here:
 - VLC_SUCCESS: the runnable is submitted
 - other: there was an error, the runnable will not be executed

> 
> > +/**
> > + * Cancel all submitted tasks having the specified id.
> > + *
> > + * If id is NULL, then cancel all tasks.
> > + *
> > + * If may_interrupt_if_running is true, then the current running tasks may
> > be + * interrupted using the interrupt() callback.
> > + *
> > + * \param executor the executor
> > + * \param id       the id of the tasks to cancel (NULL for all tasks)
> > + * \param may_interrupt_if_running indicate if the tasks currently running
> > may + *                                 be interrupted
> > + */
> > +VLC_API void
> > +vlc_executor_Cancel(vlc_executor_t *executor, void *id,
> > +                    bool may_interrupt_if_running);
> 
> Two separate external functions may be easier to read than a bool.
> 
> > diff --git a/src/misc/executor.c b/src/misc/executor.c
> > new file mode 100644
> > index 0000000000..88c7064b72
> 
> > +/**
> > + * Make sure that an interrupt thread is running and will consider the
> > + * interruption for a new task just added.
> > + */
> > +static int
> > +RequireInterruptThread(vlc_executor_t *executor)
> > +{
> > +    vlc_mutex_assert(&executor->lock);
> > +
> > +    if (executor->interrupt_thread_running)
> > +        vlc_cond_signal(&executor->interrupt_wait);
> > +    else
> > +    {
> > +        if (vlc_clone_detach(NULL, InterruptThreadRun, executor,
> > +                             VLC_THREAD_PRIORITY_LOW))
> 
> Detached threads cannot be safe in dynamically linked LibVLC. There's a risk 
> that LibVLC will be unloaded before the thread exits, crashing the process.

In vlc_executor_Delete(), the threads are awaited via a condition
variable (nothreads_wait). But yes that could be replaced by a join().
> 
> > +        {
> > +            vlc_mutex_unlock(&executor->lock);
> > +            return VLC_EGENERIC;
> 
> You probably can't do that since you're not handling errors at the call site.

Hmmm, true. I will probably create the interrupt thread from
vlc_executor_Submit() if necessary (so that I can report the error).

> > +        }
> > +
> > +        executor->interrupt_thread_running = true;
> > +    }
> > +
> > +    return VLC_SUCCESS;
> > +}