[PATCH v4 2/4] Documentation: Add Thread support page

[Laurent Pinchart]

On Mon, Aug 05, 2024 at 10:55:27AM +0100, Daniel Scally wrote:
> Morning Laurent
> 
> On 03/08/2024 23:45, Laurent Pinchart wrote:
> > Hi Dan,
> >
> > Thank you for the patch.
> >
> > On Wed, Jul 31, 2024 at 02:51:59PM +0100, Daniel Scally wrote:
> >> Move the Thread Support page and the section of the Thread class'
> >> documentation dealing with stopping threads from
> >> src/libcamera/base/thread.cpp to a dedicated .dox file at
> >> Documentation/. This is done to support the splitting of the
> >> Documentation into a public and internal version. With a separate
> >> page, references can be made to thread safety without having to
> >> include the Thread class in the doxygen run.
> >>
> >> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> >> ---
> >> Changes in v4:
> >>
> >> 	- None
> >>
> >> Changes in v3:
> >>
> >> 	- None
> >>
> >> Changes in v2:
> >>
> >> 	- New patch
> >>
> >>   Documentation/Doxyfile.in     |   4 +-
> >>   Documentation/thread.dox      | 122 +++++++++++++++++++++++++++++++++
> >>   src/libcamera/base/thread.cpp | 123 ----------------------------------
> >>   3 files changed, 125 insertions(+), 124 deletions(-)
> >>   create mode 100644 Documentation/thread.dox
> >>
> >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> >> index abafcf6c..dd1d909b 100644
> >> --- a/Documentation/Doxyfile.in
> >> +++ b/Documentation/Doxyfile.in
> >> @@ -22,7 +22,8 @@ CASE_SENSE_NAMES       = YES
> >>   QUIET                  = YES
> >>   WARN_AS_ERROR          = @WARN_AS_ERROR@
> >>   
> >> -INPUT                  = "@TOP_SRCDIR@/include/libcamera" \
> >> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> >> +                         "@TOP_SRCDIR@/include/libcamera" \
> >>                            "@TOP_SRCDIR@/src/ipa/ipu3" \
> >>                            "@TOP_SRCDIR@/src/ipa/libipa" \
> >>                            "@TOP_SRCDIR@/src/libcamera" \
> >> @@ -31,6 +32,7 @@ INPUT                  = "@TOP_SRCDIR@/include/libcamera" \
> >>   
> >>   FILE_PATTERNS          = *.c \
> >>                            *.cpp \
> >> +                         *.dox \
> >>                            *.h
> >>   
> >>   RECURSIVE              = YES
> >> diff --git a/Documentation/thread.dox b/Documentation/thread.dox
> >> new file mode 100644
> >> index 00000000..805a864e
> >> --- /dev/null
> >> +++ b/Documentation/thread.dox
> >> @@ -0,0 +1,122 @@
> >> +/**
> >> + * \page thread Thread Support
> >> + *
> >> + * libcamera supports multi-threaded applications through a threading model that
> >> + * sets precise rules to guarantee thread-safe usage of the API. Additionally,
> >> + * libcamera makes internal use of threads, and offers APIs that simplify
> >> + * interactions with application threads. Careful compliance with the threading
> >> + * model will ensure avoidance of race conditions.
> >> + *
> >> + * Every thread created by libcamera is associated with an instance of the
> >> + * Thread class. Those threads run an internal event loop by default to
> >
> > I've left a review comment on v3 that seems to have been unaddressed.
> > I'll copy it here:
> 
> Oops, sorry - no idea how I missed that.

Ei se mitään.

> > I think moving the thread-safety part of the documentation to a
> > dedicated file can make sense, but you're moving too much here. The
> > public documentation shouldn't reference the Thread class, or any of the
> > other internal details. The public part should focus on the public API.
> 
> OK. I'll look at reworking the page to focus on the thread safety section without reference to the 
> Thread class. Most of the references from public classes are to the thread safety section, but we do 
> also refer to the "Stopping Threads" section twice from object.cpp (in the documentation for 
> Object::postMessage() and Object::invokeMethod()), which is potentially more problematic as that 
> part is intrinsically related to the Thread class...possibly those "See \ref thread-stop for 
> additional information" call-sites can simply be removed, or hidden for the public run? It seems 
> unlikely that public users would need to see them. What do you think?

The Object class is included in the public libcamera API as it's a base
of public classes, but I don't think it should actually be included in
the public API documentation. It's an implementation detail, and
applications are not supposed to call any of the Object member
functions.

I'm about to post a v5 of this series that hides the Object and
Extensible classes from the public API documentation.

> >> + * dispatch events to objects. Additionally, the main thread of the application
> >> + * (defined as the thread that calls CameraManager::start()) is also associated
> >> + * with a Thread instance, but has no event loop accessible to libcamera. Other
> >> + * application threads are not visible to libcamera.
> >> + *
> >> + * \section thread-objects Threads and Objects
> >> + *
> >> + * Instances of the Object class and all its derived classes are thread-aware
> >> + * and are bound to the thread they are created in. They are said to *live* in
> >> + * a thread, and they interact with the event loop of their thread for the
> >> + * purpose of message passing and signal delivery. Messages posted to the
> >> + * object with Object::postMessage() will be delivered from the event loop of
> >> + * the thread that the object lives in. Signals delivered to the object, unless
> >> + * explicitly connected with ConnectionTypeDirect, will also be delivered from
> >> + * the object thread's event loop.
> >> + *
> >> + * All Object instances created internally by libcamera are bound to internal
> >> + * threads. As objects interact with thread event loops for proper operation,
> >> + * creating an Object instance in a thread that has no internal event loop (such
> >> + * as the main application thread, or libcamera threads that have a custom main
> >> + * loop), prevents some features of the Object class from being used. See
> >> + * Thread::exec() for more details.
> >> + *
> >> + * \section thread-signals Threads and Signals
> >> + *
> >> + * When sent to a receiver that does not inherit from the Object class, signals
> >> + * are delivered synchronously in the thread of the sender. When the receiver
> >> + * inherits from the Object class, delivery is by default asynchronous if the
> >> + * sender and receiver live in different threads. In that case, the signal is
> >> + * posted to the receiver's message queue and will be delivered from the
> >> + * receiver's event loop, running in the receiver's thread. This mechanism can
> >> + * be overridden by selecting a different connection type when calling
> >> + * Signal::connect().
> >> + *
> >> + * \section thread-reentrancy Reentrancy and Thread-Safety
> >> + *
> >> + * Through the documentation, several terms are used to define how classes and
> >> + * their member functions can be used from multiple threads.
> >> + *
> >> + * - A **reentrant** function may be called simultaneously from multiple
> >> + *   threads if and only if each invocation uses a different instance of the
> >> + *   class. This is the default for all member functions not explictly marked
> >> + *   otherwise.
> >> + *
> >> + * - \anchor thread-safe A **thread-safe** function may be called
> >> + *   simultaneously from multiple threads on the same instance of a class. A
> >> + *   thread-safe function is thus reentrant. Thread-safe functions may also be
> >> + *   called simultaneously with any other reentrant function of the same class
> >> + *   on the same instance.
> >> + *
> >> + * - \anchor thread-bound A **thread-bound** function may be called only from
> >> + *   the thread that the class instances lives in (see section \ref
> >> + *   thread-objects). For instances of classes that do not derive from the
> >> + *   Object class, this is the thread in which the instance was created. A
> >> + *   thread-bound function is not thread-safe, and may or may not be reentrant.
> >> + *
> >> + * Neither reentrancy nor thread-safety, in this context, mean that a function
> >> + * may be called simultaneously from the same thread, for instance from a
> >> + * callback invoked by the function. This may deadlock and isn't allowed unless
> >> + * separately documented.
> >> + *
> >> + * A class is defined as reentrant, thread-safe or thread-bound if all its
> >> + * member functions are reentrant, thread-safe or thread-bound respectively.
> >> + * Some member functions may additionally be documented as having additional
> >> + * thread-related attributes.
> >> + *
> >> + * Most classes are reentrant but not thread-safe, as making them fully
> >> + * thread-safe would incur locking costs considered prohibitive for the
> >> + * expected use cases.
> >> + *
> >> + * \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.
> >> + */
> >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> >> index 72733431..1962dd32 100644
> >> --- a/src/libcamera/base/thread.cpp
> >> +++ b/src/libcamera/base/thread.cpp
> >> @@ -20,88 +20,6 @@
> >>   #include <libcamera/base/mutex.h>
> >>   #include <libcamera/base/object.h>
> >>   
> >> -/**
> >> - * \page thread Thread Support
> >> - *
> >> - * libcamera supports multi-threaded applications through a threading model that
> >> - * sets precise rules to guarantee thread-safe usage of the API. Additionally,
> >> - * libcamera makes internal use of threads, and offers APIs that simplify
> >> - * interactions with application threads. Careful compliance with the threading
> >> - * model will ensure avoidance of race conditions.
> >> - *
> >> - * Every thread created by libcamera is associated with an instance of the
> >> - * Thread class. Those threads run an internal event loop by default to
> >> - * dispatch events to objects. Additionally, the main thread of the application
> >> - * (defined as the thread that calls CameraManager::start()) is also associated
> >> - * with a Thread instance, but has no event loop accessible to libcamera. Other
> >> - * application threads are not visible to libcamera.
> >> - *
> >> - * \section thread-objects Threads and Objects
> >> - *
> >> - * Instances of the Object class and all its derived classes are thread-aware
> >> - * and are bound to the thread they are created in. They are said to *live* in
> >> - * a thread, and they interact with the event loop of their thread for the
> >> - * purpose of message passing and signal delivery. Messages posted to the
> >> - * object with Object::postMessage() will be delivered from the event loop of
> >> - * the thread that the object lives in. Signals delivered to the object, unless
> >> - * explicitly connected with ConnectionTypeDirect, will also be delivered from
> >> - * the object thread's event loop.
> >> - *
> >> - * All Object instances created internally by libcamera are bound to internal
> >> - * threads. As objects interact with thread event loops for proper operation,
> >> - * creating an Object instance in a thread that has no internal event loop (such
> >> - * as the main application thread, or libcamera threads that have a custom main
> >> - * loop), prevents some features of the Object class from being used. See
> >> - * Thread::exec() for more details.
> >> - *
> >> - * \section thread-signals Threads and Signals
> >> - *
> >> - * When sent to a receiver that does not inherit from the Object class, signals
> >> - * are delivered synchronously in the thread of the sender. When the receiver
> >> - * inherits from the Object class, delivery is by default asynchronous if the
> >> - * sender and receiver live in different threads. In that case, the signal is
> >> - * posted to the receiver's message queue and will be delivered from the
> >> - * receiver's event loop, running in the receiver's thread. This mechanism can
> >> - * be overridden by selecting a different connection type when calling
> >> - * Signal::connect().
> >> - *
> >> - * \section thread-reentrancy Reentrancy and Thread-Safety
> >> - *
> >> - * Through the documentation, several terms are used to define how classes and
> >> - * their member functions can be used from multiple threads.
> >> - *
> >> - * - A **reentrant** function may be called simultaneously from multiple
> >> - *   threads if and only if each invocation uses a different instance of the
> >> - *   class. This is the default for all member functions not explictly marked
> >> - *   otherwise.
> >> - *
> >> - * - \anchor thread-safe A **thread-safe** function may be called
> >> - *   simultaneously from multiple threads on the same instance of a class. A
> >> - *   thread-safe function is thus reentrant. Thread-safe functions may also be
> >> - *   called simultaneously with any other reentrant function of the same class
> >> - *   on the same instance.
> >> - *
> >> - * - \anchor thread-bound A **thread-bound** function may be called only from
> >> - *   the thread that the class instances lives in (see section \ref
> >> - *   thread-objects). For instances of classes that do not derive from the
> >> - *   Object class, this is the thread in which the instance was created. A
> >> - *   thread-bound function is not thread-safe, and may or may not be reentrant.
> >> - *
> >> - * Neither reentrancy nor thread-safety, in this context, mean that a function
> >> - * may be called simultaneously from the same thread, for instance from a
> >> - * callback invoked by the function. This may deadlock and isn't allowed unless
> >> - * separately documented.
> >> - *
> >> - * A class is defined as reentrant, thread-safe or thread-bound if all its
> >> - * member functions are reentrant, thread-safe or thread-bound respectively.
> >> - * Some member functions may additionally be documented as having additional
> >> - * thread-related attributes.
> >> - *
> >> - * Most classes are reentrant but not thread-safe, as making them fully
> >> - * thread-safe would incur locking costs considered prohibitive for the
> >> - * expected use cases.
> >> - */
> >> -
> >>   /**
> >>    * \file base/thread.h
> >>    * \brief Thread support
> >> @@ -217,47 +135,6 @@ 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.
> >>    */
> >>   
> >>   /**

-- 
Regards,

Laurent Pinchart