[libcamera-devel] [PATCH 09/19] libcamera: signal: Make slots list private
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 20 15:00:06 CET 2020
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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200120/79c8b2ec/attachment.sig>
More information about the libcamera-devel
mailing list