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

[Laurent Pinchart]

Hi Umang,

On Mon, May 24, 2021 at 08:47:00AM +0530, Umang Jain wrote:
> Hi Laurent,
> 
> Thanks for the work.
> 
> Great to read the contexts for message deliveries you have mentioned. 
> However, how does one decide whether they need to ensure message 
> delivery before exit() ? What would be the applicable use-cases to that 
> context? (Just curious)

It depends if the user of the thread needs a guarantee that all messages
will be processed. That doesn't really answer your question...

Let's take a practical example, posting post-processing work to a thread
(which we should do quite soon, JPEG compression in the request
completion handler isn't a good idea). When stopping the camera (HAL
.close() or .flush()), if the HAL required for proper operation that the
thread reports completion of all post-processing requests, then we'd
need to ensure message delivery. On the other hand, the HAL could also
stop the thread, and then look into its own queue of requests that are
waiting for post-processing. Any request in that queue would then be
considered as cancelled are reported as such to the camera service. In
that case, we wouldn't require the thread to process all posted
messages.

> On 5/23/21 8:01 AM, 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>
>
> Reviewed-by: Umang Jain <umang.jain 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
> > + * request and return, at which points the thread will stop.
> > + *
> > + * For threads running exec(), the exit() function is used to request the thread
> > + * 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:
> > + *
> > + * - 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