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

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Mon, May 24, 2021 at 12:44 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

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

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


> > > ---
> > >   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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210524/6f731605/attachment-0001.htm>