| Message ID | 20200120002437.6633-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, nit: I would move this before the patch that adds locking to this functions. This change alone does not change how things work today On Mon, Jan 20, 2020 at 02:24:27AM +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 s/list list/list > 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> > --- > 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(). > + */ Shouldn't we lock as well here ? Isn't there a small window during the move of signals_ ? > + 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); > +} > + I assume this will all get locked later The patch itself is fine, with or without locking squashed in Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > +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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Jan 20, 2020 at 03:00:06PM +0100, Jacopo Mondi wrote: > Hi Laurent, > > nit: I would move this before the patch that adds locking to this > functions. I'm confused, isn't it already the case ? > This change alone does not change how things work today It fixes the bug pointed out by patch 08/19, doesn't it ? > On Mon, Jan 20, 2020 at 02:24:27AM +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 > > s/list list/list > > > 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> > > --- > > 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(). > > + */ > > Shouldn't we lock as well here ? Isn't there a small window during the move of > signals_ ? There is, but the only other place that touches signals_ is Object::connect() and Object::disconnect(). If you call those methods (through Signal::connect() or Signal::disconnect()) from another thread while the Object instance is being deleted, you're in bigger trouble anyway :-) No locking is needed here in my opinion as the caller should guarantee that the Object instance won't be deleted while the instance is still being accessed. The only exception is the call to Object::disconnect() from SignalBase::disconnect(), called below, but that runs in the same thread. > > + 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); > > +} > > + > > I assume this will all get locked later Yes, in patch 12/19, this call will be protected by a global mutex. As explained in that patch we may want to use finer-grain locks in the future, but I think that can then be built on top. > The patch itself is fine, with or without locking squashed in I'd rather keep the locking separate, on top of this patch, as it's part of the threading model and should be reviewed separately from the required refactoring. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > +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
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@ideasonboard.com> With the 'lists list' typo pointed out by Jacopo fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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
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@ideasonboard.com> --- 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(-)