libcamera: signal: Make slots list private

The slots list is touched from most of the Signal template functions. In
order to prepare for thread-safety, move handling of the 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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

include/libcamera/object.h

@@ -48,9 +48,7 @@ protected:
 	virtual void message(Message *msg);
 
 private:
-	template<typename... Args>
+	friend class SignalBase;
-	friend class Signal;
-	friend class BoundMethodBase;
 	friend class Thread;
 
 	void notifyThreadMove();

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(Object *object);
-	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;
-			}
-		}
-	}
 
 protected:
-	friend class Object;
+	using SlotList = std::list<BoundMethodBase *>;
-	std::list<BoundMethodBase *> slots_;
+
+	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_) {
+		disconnect();
-			Object *object = slot->object();
-			if (object)
-				object->disconnect(this);
-			delete slot;
-		}
 	}
 
 #ifndef __DOXYGEN__
@@ -59,8 +50,7 @@ public:
 		     ConnectionType type = ConnectionTypeAuto)
 	{
 		Object *object = static_cast<Object *>(obj);
-		object->connect(this);
+		SignalBase::connect(new BoundMethodMember<T, void, Args...>(obj, object, func, type));
-		slots_.push_back(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_)
+		SignalBase::disconnect([](SlotList::iterator &iter) {
-			delete slot;
+			return true;
-		slots_.clear();
+		});
 	}
 
 	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) &&
+			return static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func);
-			    static_cast<BoundMethodMember<T, R, Args...> *>(slot)->match(func)) {
+		});
-				iter = slots_.erase(iter);
-				delete slot;
-			} else {
-				++iter;
-			}
-		}
 	}
 
 	template<typename R>
 	void disconnect(R (*func)(Args...))
 	{
-		for (auto iter = slots_.begin(); iter != slots_.end(); ) {
+		SignalBase::disconnect([func](SlotList::iterator &iter) {
-			BoundMethodArgs<R, Args...> *slot = *iter;
+			BoundMethodArgs<R, Args...> *slot =
-			if (slot->match(nullptr) &&
+				static_cast<BoundMethodArgs<R, Args...> *>(*iter);
-			    static_cast<BoundMethodStatic<R, Args...> *>(slot)->match(func)) {
+
-				iter = slots_.erase(iter);
+			if (!slot->match(nullptr))
-				delete slot;
+				return false;
-			} else {
+
-				++iter;
+			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...);
 	}
 };

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

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