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

[Rémi Denis-Courmont]

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.

> + * \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.

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

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

It is unlikely that callers will (or even should have to) handle errors well 
at this point.

> +/**
> + * 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.

> +        {
> +            vlc_mutex_unlock(&executor->lock);
> +            return VLC_EGENERIC;

You probably can't do that since you're not handling errors at the call site.

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

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/