[libcamera-devel] [PATCH 12/19] libcamera: signal: Make connection and disconnection thread-safe

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-01-20 02:24:30 +0200, Laurent Pinchart wrote:
> Make the signal connection and disconnection thread-safe, and document
> them as such. This is required to make objects connectable from
> different threads.
> 
> The connect(), disconnect() and emit() methods are now all protected by
> a global mutex, which may generate a high lock contention. This could be
> improved with finer-grained locks or with a pool of mutexes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/libcamera/signal.cpp | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> index 9bb7eca8ed6e..6eab1fa74d42 100644
> --- a/src/libcamera/signal.cpp
> +++ b/src/libcamera/signal.cpp
> @@ -7,6 +7,8 @@
>  
>  #include <libcamera/signal.h>
>  
> +#include "thread.h"
> +
>  /**
>   * \file signal.h
>   * \brief Signal & slot implementation
> @@ -14,8 +16,21 @@
>  
>  namespace libcamera {
>  
> +namespace {
> +
> +/*
> + * Mutex to protect the SignalBase::slots_ and Object::signals_ lists. If lock
> + * contention needs to be decreased, this could be replaced with locks in
> + * Object and SignalBase, or with a mutex pool.
> + */
> +Mutex signalsLock;
> +
> +} /* namespace */
> +
>  void SignalBase::connect(BoundMethodBase *slot)
>  {
> +	MutexLocker locker(signalsLock);
> +
>  	Object *object = slot->object();
>  	if (object)
>  		object->connect(this);
> @@ -31,6 +46,8 @@ void SignalBase::disconnect(Object *object)
>  
>  void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)
>  {
> +	MutexLocker locker(signalsLock);
> +
>  	for (auto iter = slots_.begin(); iter != slots_.end(); ) {
>  		if (match(iter)) {
>  			Object *object = (*iter)->object();
> @@ -47,6 +64,7 @@ void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)
>  
>  SignalBase::SlotList SignalBase::slots()
>  {
> +	MutexLocker locker(signalsLock);
>  	return slots_;
>  }
>  
> @@ -99,23 +117,31 @@ SignalBase::SlotList SignalBase::slots()
>   * disconnected from the \a func slot of \a object when \a object is destroyed.
>   * Otherwise the caller shall disconnect signals manually before destroying \a
>   * object.
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
>   * \fn Signal::connect(R (*func)(Args...))
>   * \brief Connect the signal to a static function slot
>   * \param[in] func The slot static function
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
>   * \fn Signal::disconnect()
>   * \brief Disconnect the signal from all slots
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
>   * \fn Signal::disconnect(T *object)
>   * \brief Disconnect the signal from all slots of the \a object
>   * \param[in] object The object pointer whose slots to disconnect
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
> @@ -123,12 +149,16 @@ SignalBase::SlotList SignalBase::slots()
>   * \brief Disconnect the signal from the \a object slot member function \a func
>   * \param[in] object The object pointer whose slots to disconnect
>   * \param[in] func The slot member function to disconnect
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
>   * \fn Signal::disconnect(R (*func)(Args...))
>   * \brief Disconnect the signal from the slot static function \a func
>   * \param[in] func The slot static function to disconnect
> + *
> + * \context This function is \threadsafe.
>   */
>  
>  /**
> @@ -141,6 +171,9 @@ SignalBase::SlotList SignalBase::slots()
>   * function are passed to the slot functions unchanged. If a slot modifies one
>   * of the arguments (when passed by pointer or reference), the modification is
>   * thus visible to all subsequently called slots.
> + *
> + * This function is not \threadsafe, but thread-safety is guaranteed against
> + * concurrent connect() and disconnect() calls.
>   */
>  
>  } /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund