[libcamera-devel] [PATCH 11/19] libcamera: Document thread-safety attributes of core classes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 20 19:04:43 CET 2020
Hi Jacopo,
On Mon, Jan 20, 2020 at 03:58:26PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 20, 2020 at 02:24:29AM +0200, Laurent Pinchart wrote:
> > Define the thread-safety attributes of the classes and methods that are
> > either thread-safe or thread-bound. The CameraManager, Camera and
> > PipelineHandler will be addressed separately.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/device_enumerator.cpp | 2 ++
> > src/libcamera/event_notifier.cpp | 2 ++
> > src/libcamera/ipc_unixsocket.cpp | 2 ++
> > src/libcamera/object.cpp | 10 ++++++++--
> > src/libcamera/thread.cpp | 6 +++++-
> > src/libcamera/timer.cpp | 20 ++++++++++++--------
> > src/libcamera/v4l2_videodevice.cpp | 2 ++
> > 7 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> > index a8b5c90f5a5d..64cdc132308a 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -190,6 +190,8 @@ DeviceEnumerator::~DeviceEnumerator()
> > * with a warning message logged, without returning an error. Only errors that
> > * prevent enumeration altogether shall be fatal.
> > *
> > + * \context This function is \threadbound.
> > + *
>
> I'm trying to grasp why enumeration is thread bound
Same as for IPCUnixSocket below, because it creates an EventNotifier for
the udev-based enumerator.
> > * \return 0 on success or a negative error code otherwise
> > */
> >
> > diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> > index 4326b0b413e2..a9be686f79ae 100644
> > --- a/src/libcamera/event_notifier.cpp
> > +++ b/src/libcamera/event_notifier.cpp
> > @@ -99,6 +99,8 @@ EventNotifier::~EventNotifier()
> > *
> > * This function enables or disables the notifier. A disabled notifier ignores
> > * events and does not emit the \ref activated signal.
> > + *
> > + * \context This function is \threadbound.
> > */
> > void EventNotifier::setEnabled(bool enable)
> > {
> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > index eb1a50239188..6e5cab894a93 100644
> > --- a/src/libcamera/ipc_unixsocket.cpp
> > +++ b/src/libcamera/ipc_unixsocket.cpp
> > @@ -62,6 +62,8 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)
> > * communication method. The remote side then instantiates a socket, and binds
> > * it to the other side by passing the file descriptor to bind(). At that point
> > * the channel is operation and communication is bidirectional and symmmetrical.
> > + *
> > + * \context This class is \threadbound.
>
> I'm wondering if a "why" statement would be beneficial. I had to go
> and look to find out the class instantiates an EventNotifier bound to
> the current thread.
The idea here is to document the API for the callers, not the internal
implementation. The documentation thus tells that callers shall only
call the functions of the IPCUnixSocket class from the thread that the
instance is bound to, otherwise behaviour is undefined. If the actual
implementation makes some functions thread-safe, that doesn't change the
API contract, it's an implementation detail. The documentation also
tells the developers that the implementation of IPCUnixSocket does not
have to be thread-safe and may be thread-bound. Same for the device
enumerator above, we know that some device enumerators may need to
create a notifier in their enumerate() function, we thus document the
function as thread-bound. If the implementation isn't thread-bound,
that's not an issue, it doesn't change the contract between the caller
and the function, and allows future changes to the function to be
thread-bound.
> > */
> >
> > IPCUnixSocket::IPCUnixSocket()
> > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> > index f2a8be172547..99c3bf9a709b 100644
> > --- a/src/libcamera/object.cpp
> > +++ b/src/libcamera/object.cpp
> > @@ -110,6 +110,8 @@ Object::~Object()
> > * Messages are delivered through the thread's event loop. If the thread is not
> > * running its event loop the message will not be delivered until the event
> > * loop gets started.
> > + *
> > + * \context This function is \threadsafe.
> > */
> > void Object::postMessage(std::unique_ptr<Message> msg)
> > {
> > @@ -162,6 +164,8 @@ void Object::message(Message *msg)
> > * are passed untouched. The caller shall ensure that any pointer argument
> > * remains valid until the method is invoked.
> > *
> > + * \context This function is \threadsafe.
> > + *
>
> they will be since next patch :)
You're right :-) Should I move this to the next patch ?
> > * \return For connection types ConnectionTypeDirect and
> > * ConnectionTypeBlocking, return the return value of the invoked method. For
> > * connection type ConnectionTypeQueued, return a default-constructed R value.
> > @@ -170,6 +174,7 @@ void Object::message(Message *msg)
> > /**
> > * \fn Object::thread()
> > * \brief Retrieve the thread the object is bound to
> > + * \context This function is \threadsafe.
> > * \return The thread the object is bound to
> > */
> >
> > @@ -178,8 +183,7 @@ void Object::message(Message *msg)
> > * \param[in] thread The target thread
> > *
> > * This method moves the object and all its children from the current thread to
> > - * the new \a thread. It shall be called from the thread in which the object
> > - * currently lives, otherwise the behaviour is undefined.
> > + * the new \a thread.
> > *
> > * Before the object is moved, a Message::ThreadMoveMessage message is sent to
> > * it. The message() method can be reimplement in derived classes to be notified
> > @@ -187,6 +191,8 @@ void Object::message(Message *msg)
> > *
> > * Moving an object that has a parent is not allowed, and causes undefined
> > * behaviour.
> > + *
> > + * \context This function is thread-bound.
> > */
> > void Object::moveToThread(Thread *thread)
> > {
> > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp
> > index c1698d469a6c..0cf6ff691315 100644
> > --- a/src/libcamera/thread.cpp
> > +++ b/src/libcamera/thread.cpp
> > @@ -40,7 +40,9 @@
> > *
> > * - \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 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.
>
> Is this intentionally not squashed in the previous patch ?
Correct :-) I'll fix it.
> > *
> > * - \anchor thread-bound A **thread-bound** function may be called only from
> > * the thread that the class instances lives in (see section \ref
> > @@ -220,6 +222,8 @@ ThreadData *ThreadData::current()
> > * called. A custom event dispatcher may be installed with
> > * setEventDispatcher(), otherwise a poll-based event dispatcher is used. This
> > * behaviour can be overriden by overloading the run() method.
> > + *
> > + * \context This class is \threadsafe.
> > */
> >
> > /**
> > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > index 4c68883204e8..24da51524efb 100644
> > --- a/src/libcamera/timer.cpp
> > +++ b/src/libcamera/timer.cpp
> > @@ -67,16 +67,18 @@ Timer::~Timer()
> > * \brief Start or restart the timer with a timeout of \a msec
> > * \param[in] msec The timer duration in milliseconds
> > *
> > - * This method shall be called from the thread the timer is associated with. If
> > - * the timer is already running it will be stopped and restarted.
> > + * If the timer is already running it will be stopped and restarted.
> > + *
> > + * \context This function is \threadbound.
> > */
> >
> > /**
> > * \brief Start or restart the timer with a timeout of \a duration
> > * \param[in] duration The timer duration in milliseconds
> > *
> > - * This method shall be called from the thread the timer is associated with. If
> > - * the timer is already running it will be stopped and restarted.
> > + * If the timer is already running it will be stopped and restarted.
> > + *
> > + * \context This function is \threadbound.
> > */
> > void Timer::start(std::chrono::milliseconds duration)
> > {
> > @@ -87,8 +89,9 @@ void Timer::start(std::chrono::milliseconds duration)
> > * \brief Start or restart the timer with a \a deadline
> > * \param[in] deadline The timer deadline
> > *
> > - * This method shall be called from the thread the timer is associated with. If
> > - * the timer is already running it will be stopped and restarted.
> > + * If the timer is already running it will be stopped and restarted.
> > + *
> > + * \context This function is \threadbound.
> > */
> > void Timer::start(std::chrono::steady_clock::time_point deadline)
> > {
> > @@ -115,8 +118,9 @@ void Timer::start(std::chrono::steady_clock::time_point deadline)
> > * After this function returns the timer is guaranteed not to emit the
> > * \ref timeout signal.
> > *
> > - * This method shall be called from the thread the timer is associated with. If
> > - * the timer is not running this function performs no operation.
> > + * If the timer is not running this function performs no operation.
> > + *
> > + * \context This function is \threadbound.
> > */
> > void Timer::stop()
> > {
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 51be1dcd7fff..cb166c795abd 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -404,6 +404,8 @@ const std::string V4L2DeviceFormat::toString() const
> > *
> > * Upon destruction any device left open will be closed, and any resources
> > * released.
> > + *
> > + * \context This class is \threadbound.
> > */
> >
> > /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list