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

[Jacopo Mondi]

Hi LAurent,

On Mon, May 24, 2021 at 12:34:38PM +0300, Laurent Pinchart wrote:
> 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().
>

Threads that do not override run() will use the default base
implementation that just run exec(). That's what I meant.

However, details
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

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