[libcamera-devel] [PATCH 09/19] libcamera: signal: Make slots list private
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 20 18:48:11 CET 2020
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 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_ ?
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 at 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list