[libcamera-devel] [PATCH 4/4] libcamera: thread: Document race condition at stop time

[Laurent Pinchart]

Hi Jacopo,

On Mon, May 24, 2021 at 11:06:21AM +0200, Jacopo Mondi wrote:
> On Sun, May 23, 2021 at 05:31:55AM +0300, Laurent Pinchart wrote:
> > When a thread stops, messages may be left in its message queue. Document
> > this in details, with a way to force processing of pending messages when
> > the thread is stopped.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/object.cpp |  8 ++++++++
> >  src/libcamera/thread.cpp | 44 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> > index cd83c684b989..5e6b73f9af84 100644
> > --- a/src/libcamera/object.cpp
> > +++ b/src/libcamera/object.cpp
> > @@ -155,6 +155,10 @@ void Object::deleteLater()
> >   * running its event loop the message will not be delivered until the event
> >   * loop gets started.
> >   *
> > + * Due to their asynchronous nature, threads do not provide any guarantee that
> > + * all posted messages are delivered before the thread is stopped. See
> > + * \ref thread-stop for additional information.
> > + *
> >   * \context This function is \threadsafe.
> >   */
> >  void Object::postMessage(std::unique_ptr<Message> msg)
> > @@ -212,6 +216,10 @@ void Object::message(Message *msg)
> >   * are passed untouched. The caller shall ensure that any pointer argument
> >   * remains valid until the method is invoked.
> >   *
> > + * Due to the asynchronous nature of threads, functions invoked asynchronously
> > + * with the ConnectionTypeQueued type are not guaranteed to be called before
> > + * the thread is stopped. See \ref thread-stop for additional information.
> > + *
> >   * \context This function is \threadsafe.
> >   *
> >   * \return For connection types ConnectionTypeDirect and
> > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp
> > index d59e43966d26..91e4737ad032 100644
> > --- a/src/libcamera/thread.cpp
> > +++ b/src/libcamera/thread.cpp
> > @@ -221,6 +221,47 @@ ThreadData *ThreadData::current()
> >   * called. The event loop dispatches events (messages, notifiers and timers)
> >   * sent to the objects living in the thread. This behaviour can be modified by
> >   * overriding the run() function.
> > + *
> > + * \section thread-stop Stopping Threads
> > + *
> > + * Threads can't be forcibly stopped. Instead, a thread user first requests the
> > + * thread to exit and then waits for the thread's main function to react to the
> 
> isn't it better to say 'the thread's event loop to stop' in place of
> the 'main function to react' ?

A thread doesn't necessarily have an event loop, but it always has a
main function.

> > + * request and return, at which points the thread will stop.
> > + *
> > + * For threads running exec(), the exit() function is used to request the thread
> 
> Threads do not exactly run exec() directly, I guess the real
> difference is made by the fact the Thread (sub-class) overrides run()
> or not, don't it ?
>
> What about something similar to:
> 
>       For Thread derived classes that do not overrider the run()
>       function, the exit() function is used to interrupt the thread's

More precisely, it's about whether the run() function will run exec(),
either using the default implementation, or in an overridden run()
function that calls exec() manually. It's thus not only about threads
that don't override run().

>       even loop and exit. Derived classes that overrides the run()
>       function implementation might provide a custom mechanism to
>       handle the event loop exit sequence. In either cases, the wait()
>       function shall be called to wait for the underlying thread to
>       stop().
> 
> > + * to exit. For threads subclassing the Thread class and implementing a custom
> > + * run() function, a subclass-specific mechanism shall be provided. In either
> > + * case, the wait() function shall be called to wait for the thread to stop.
> > + *
> > + * Due to their asynchronous nature, threads are subject to race conditions when
> > + * they stop. This is of particular importance for messages posted to the thread
> > + * with postMessage() (and the other mechanisms that rely on it, such as
> > + * Object::invokeMethod() or asynchronous signal delivery). To understand the
> > + * issues, three contexts need to be considered:
> 
> You know, I would move this paragraph (probelem statement) before the
> previous one.

I had that before, but I wasn't entirely happy with it. I decided to
first explain how to stop a thread, and then focus on one specific
aspect of stop issues, which is message handling. Talking about the
messages issue first, and then explaining how to stop threads, makes it
sound like the stop procedure is specific to message handling.

> Then explain threads sub-class might or might not override run()
> 
> The provide the example here below which is very clear.
> 
> > + *
> > + * - The worker is the Thread performing work and being instructed to stop.
> > + * - The controller is the context which instructs the worker thread to stop.
> > + * - The other contexts are any threads other than the worker and controller
> > + *   that interact with the worker thread.
> > + *
> > + * Messages posted to the worker thread from the controller context before
> > + * calling exit() are queued to the thread's message queue, and the Thread class
> > + * offers no guarantee that those messages will be processed before the thread
> > + * stops. This allows threads to stop fast.
> > + *
> > + * A thread that requires delivery of messages posted from the controller
> > + * context before exit() should reimplement the run() function and call
> > + * dispatchMessages() after exec().
> > + *
> > + * Messages posted to the worker thread from the other contexts are asynchronous
> > + * with respect to the exit() call from the controller context. There is no
> > + * guarantee as to whether those messages will be processed or not before the
> > + * thread stops.
> > + *
> > + * Messages that are not processed will stay in the queue, in the exact same way
> > + * as messages posted after the thread has stopped. They will be processed when
> > + * the thread is restarted. If the thread is never restarted, they will be
> > + * deleted without being processed when the Thread instance is destroyed.
> >   */
> >
> >  /**
> > @@ -480,6 +521,9 @@ EventDispatcher *Thread::eventDispatcher()
> >   * running its event loop the message will not be delivered until the event
> >   * loop gets started.
> >   *
> > + * When the thread is stopped, posted messages may not have all been processed.
> > + * See \ref thread-stop for additional information.
> > + *
> >   * If the \a receiver is not bound to this thread the behaviour is undefined.
> >   *
> >   * \sa exec()

-- 
Regards,

Laurent Pinchart