[libcamera-devel] [PATCH 09/19] libcamera: signal: Make slots list private

Niklas Söderlund niklas.soderlund at ragnatech.se
Wed Jan 22 16:26:21 CET 2020


Hi Laurent,

Thanks for your work.

On 2020-01-20 02:24:27 +0200, Laurent Pinchart wrote:
> The slots list is touched from most of the Signal template functions. In
> order to prepare for thread-safety, move handling of the lists list to a
> small number of non-template functions in the SignalBase class.
> 
> This incidently fixes a bug in signal disconnection handling where the
> signal wasn't removed from the object's signals list, as pointed out by
> the signals unit test.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

With the 'lists list' typo pointed out by Jacopo fixed,

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

> ---
>  include/libcamera/object.h |  4 +-
>  include/libcamera/signal.h | 88 ++++++++++++++++----------------------
>  src/libcamera/object.cpp   |  7 ++-
>  src/libcamera/signal.cpp   | 36 ++++++++++++++++
>  4 files changed, 81 insertions(+), 54 deletions(-)
> 
> diff --git a/include/libcamera/object.h b/include/libcamera/object.h
> index 9344af30bc79..4d16f3f2b610 100644
> --- a/include/libcamera/object.h
> +++ b/include/libcamera/object.h
> @@ -48,9 +48,7 @@ protected:
>  	virtual void message(Message *msg);
>  
>  private:
> -	template<typename... Args>
> -	friend class Signal;
> -	friend class BoundMethodBase;
> +	friend class SignalBase;
>  	friend class Thread;
>  
>  	void notifyThreadMove();
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> index 432d95d0ed1c..c13bb30f0636 100644
> --- a/include/libcamera/signal.h
> +++ b/include/libcamera/signal.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_SIGNAL_H__
>  #define __LIBCAMERA_SIGNAL_H__
>  
> +#include <functional>
>  #include <list>
>  #include <type_traits>
>  #include <vector>
> @@ -19,23 +20,18 @@ namespace libcamera {
>  class SignalBase
>  {
>  public:
> -	template<typename T>
> -	void disconnect(T *obj)
> -	{
> -		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> -			BoundMethodBase *slot = *iter;
> -			if (slot->match(obj)) {
> -				iter = slots_.erase(iter);
> -				delete slot;
> -			} else {
> -				++iter;
> -			}
> -		}
> -	}
> +	void disconnect(Object *object);
>  
>  protected:
> -	friend class Object;
> -	std::list<BoundMethodBase *> slots_;
> +	using SlotList = std::list<BoundMethodBase *>;
> +
> +	void connect(BoundMethodBase *slot);
> +	void disconnect(std::function<bool(SlotList::iterator &)> match);
> +
> +	SlotList slots();
> +
> +private:
> +	SlotList slots_;
>  };
>  
>  template<typename... Args>
> @@ -45,12 +41,7 @@ public:
>  	Signal() {}
>  	~Signal()
>  	{
> -		for (BoundMethodBase *slot : slots_) {
> -			Object *object = slot->object();
> -			if (object)
> -				object->disconnect(this);
> -			delete slot;
> -		}
> +		disconnect();
>  	}
>  
>  #ifndef __DOXYGEN__
> @@ -59,8 +50,7 @@ public:
>  		     ConnectionType type = ConnectionTypeAuto)
>  	{
>  		Object *object = static_cast<Object *>(obj);
> -		object->connect(this);
> -		slots_.push_back(new BoundMethodMember<T, void, Args...>(obj, object, func, type));
> +		SignalBase::connect(new BoundMethodMember<T, void, Args...>(obj, object, func, type));
>  	}
>  
>  	template<typename T, typename R, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>
> @@ -69,63 +59,62 @@ public:
>  #endif
>  	void connect(T *obj, R (T::*func)(Args...))
>  	{
> -		slots_.push_back(new BoundMethodMember<T, R, Args...>(obj, nullptr, func));
> +		SignalBase::connect(new BoundMethodMember<T, R, Args...>(obj, nullptr, func));
>  	}
>  
>  	template<typename R>
>  	void connect(R (*func)(Args...))
>  	{
> -		slots_.push_back(new BoundMethodStatic<R, Args...>(func));
> +		SignalBase::connect(new BoundMethodStatic<R, Args...>(func));
>  	}
>  
>  	void disconnect()
>  	{
> -		for (BoundMethodBase *slot : slots_)
> -			delete slot;
> -		slots_.clear();
> +		SignalBase::disconnect([](SlotList::iterator &iter) {
> +			return true;
> +		});
>  	}
>  
>  	template<typename T>
>  	void disconnect(T *obj)
>  	{
> -		SignalBase::disconnect(obj);
> +		SignalBase::disconnect([obj](SlotList::iterator &iter) {
> +			return (*iter)->match(obj);
> +		});
>  	}
>  
>  	template<typename T, typename R>
>  	void disconnect(T *obj, R (T::*func)(Args...))
>  	{
> -		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +		SignalBase::disconnect([obj, func](SlotList::iterator &iter) {
>  			BoundMethodArgs<R, Args...> *slot =
>  				static_cast<BoundMethodArgs<R, Args...> *>(*iter);
> +
> +			if (!slot->match(obj))
> +				return false;
> +
>  			/*
>  			 * If the object matches the slot, the slot is
>  			 * guaranteed to be a member slot, so we can safely
>  			 * cast it to BoundMethodMember<T, Args...> to match
>  			 * func.
>  			 */
> -			if (slot->match(obj) &&
> -			    static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func)) {
> -				iter = slots_.erase(iter);
> -				delete slot;
> -			} else {
> -				++iter;
> -			}
> -		}
> +			return static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func);
> +		});
>  	}
>  
>  	template<typename R>
>  	void disconnect(R (*func)(Args...))
>  	{
> -		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> -			BoundMethodArgs<R, Args...> *slot = *iter;
> -			if (slot->match(nullptr) &&
> -			    static_cast<BoundMethodStatic<R, Args...> *>(slot)->match(func)) {
> -				iter = slots_.erase(iter);
> -				delete slot;
> -			} else {
> -				++iter;
> -			}
> -		}
> +		SignalBase::disconnect([func](SlotList::iterator &iter) {
> +			BoundMethodArgs<R, Args...> *slot =
> +				static_cast<BoundMethodArgs<R, Args...> *>(*iter);
> +
> +			if (!slot->match(nullptr))
> +				return false;
> +
> +			return static_cast<BoundMethodStatic<R, Args...> *>(slot)->match(func);
> +		});
>  	}
>  
>  	void emit(Args... args)
> @@ -134,8 +123,7 @@ public:
>  		 * Make a copy of the slots list as the slot could call the
>  		 * disconnect operation, invalidating the iterator.
>  		 */
> -		std::vector<BoundMethodBase *> slots{ slots_.begin(), slots_.end() };
> -		for (BoundMethodBase *slot : slots)
> +		for (BoundMethodBase *slot : slots())
>  			static_cast<BoundMethodArgs<void, Args...> *>(slot)->activate(args...);
>  	}
>  };
> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> index 21aad5652b38..f2a8be172547 100644
> --- a/src/libcamera/object.cpp
> +++ b/src/libcamera/object.cpp
> @@ -76,7 +76,12 @@ Object::Object(Object *parent)
>   */
>  Object::~Object()
>  {
> -	for (SignalBase *signal : signals_)
> +	/*
> +	 * Move signals to a private list to avoid concurrent iteration and
> +	 * deletion of items from Signal::disconnect().
> +	 */
> +	std::list<SignalBase *> signals(std::move(signals_));
> +	for (SignalBase *signal : signals)
>  		signal->disconnect(this);
>  
>  	if (pendingMessages_)
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> index 190033317c72..9bb7eca8ed6e 100644
> --- a/src/libcamera/signal.cpp
> +++ b/src/libcamera/signal.cpp
> @@ -14,6 +14,42 @@
>  
>  namespace libcamera {
>  
> +void SignalBase::connect(BoundMethodBase *slot)
> +{
> +	Object *object = slot->object();
> +	if (object)
> +		object->connect(this);
> +	slots_.push_back(slot);
> +}
> +
> +void SignalBase::disconnect(Object *object)
> +{
> +	disconnect([object](SlotList::iterator &iter) {
> +		return (*iter)->match(object);
> +	});
> +}
> +
> +void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)
> +{
> +	for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> +		if (match(iter)) {
> +			Object *object = (*iter)->object();
> +			if (object)
> +				object->disconnect(this);
> +
> +			delete *iter;
> +			iter = slots_.erase(iter);
> +		} else {
> +			++iter;
> +		}
> +	}
> +}
> +
> +SignalBase::SlotList SignalBase::slots()
> +{
> +	return slots_;
> +}
> +
>  /**
>   * \class Signal
>   * \brief Generic signal and slot communication mechanism
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list