[libcamera-devel] [PATCH 3/3] libcamera: signal: Disconnect signal automatically on slot deletion
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Feb 13 11:57:30 CET 2019
Hi Laurent,
Thanks for your patch^H^H^H^H^H magic,
On 2019-02-13 00:37:02 +0200, Laurent Pinchart wrote:
> When a signal is connected to a member function slot, the slot is not
> disconnected when the slot object is deleted. This can lead to calling a
> member function of a deleted object if the signal isn't disconnected
> manually by the slot object's destructor.
>
> Make signal handling easier by implementing a base Object class that
> tracks all connected signals and disconnects from them automatically
> when the object is deleted, using template specialization resolution in
> the Signal class.
>
> As inheriting from the Object class may to a too harsh requirement for
> Signal usage in applications, keep the existing behaviour working if the
> slot doesn't inherit from the Object class. We may reconsider this later
> and require all slot objects to inherit from the Object class.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
This was quiet fun to review and I still remembers how all
std::enable_if<std::is_base_of<...>> works, lets see for how long that
sticks in my head...
There are some small style fixups (see bellow) which could be moved to
2/3 but nothing to get your knickers in a twist over,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> Documentation/Doxyfile.in | 4 +-
> include/libcamera/meson.build | 1 +
> include/libcamera/object.h | 35 +++++++++++
> include/libcamera/signal.h | 115 +++++++++++++++++++++++-----------
> src/libcamera/meson.build | 1 +
> src/libcamera/object.cpp | 50 +++++++++++++++
> src/libcamera/signal.cpp | 5 ++
> test/signal.cpp | 37 +++++++++++
> 8 files changed, 211 insertions(+), 37 deletions(-)
> create mode 100644 include/libcamera/object.h
> create mode 100644 src/libcamera/object.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b78fb3a0b0b6..3e2b7fd9da0e 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -860,7 +860,9 @@ EXCLUDE_PATTERNS =
> # Note that the wildcards are matched against the file with absolute path, so to
> # exclude all test directories use the pattern */test/*
>
> -EXCLUDE_SYMBOLS = libcamera::SlotBase \
> +EXCLUDE_SYMBOLS = libcamera::SignalBase \
> + libcamera::SlotArgs \
> + libcamera::SlotBase \
> libcamera::SlotMember \
> libcamera::SlotStatic
>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 5788e9bbdf3e..3f4d1e28208b 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -5,6 +5,7 @@ libcamera_api = files([
> 'event_dispatcher.h',
> 'event_notifier.h',
> 'libcamera.h',
> + 'object.h',
> 'request.h',
> 'signal.h',
> 'stream.h',
> diff --git a/include/libcamera/object.h b/include/libcamera/object.h
> new file mode 100644
> index 000000000000..eadd41f9a41f
> --- /dev/null
> +++ b/include/libcamera/object.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * object.h - Base object
> + */
> +#ifndef __LIBCAMERA_OBJECT_H__
> +#define __LIBCAMERA_OBJECT_H__
> +
> +#include <list>
> +
> +namespace libcamera {
> +
> +class SignalBase;
> +template<typename... Args>
> +class Signal;
> +
> +class Object
> +{
> +public:
> + virtual ~Object();
> +
> +private:
> + template<typename... Args>
> + friend class Signal;
> +
> + void connect(SignalBase *signal);
> + void disconnect(SignalBase *signal);
> +
> + std::list<SignalBase *> signals_;
> +};
> +
> +}; /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_OBJECT_H__ */
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> index 85dc481d6957..434fc0109f42 100644
> --- a/include/libcamera/signal.h
> +++ b/include/libcamera/signal.h
> @@ -10,32 +10,47 @@
> #include <list>
> #include <vector>
>
> +#include <libcamera/object.h>
> +
> namespace libcamera {
>
> template<typename... Args>
> class Signal;
>
> -template<typename... Args>
> class SlotBase
> {
> public:
> - SlotBase(void *obj)
> - : obj_(obj) { }
> - virtual ~SlotBase() { }
> + SlotBase(void *obj, bool isObject)
> + : obj_(obj), isObject_(isObject) {}
> + virtual ~SlotBase() {}
The s/{ }/{}/ could be moved to 2/3.
>
> - virtual void invoke(Args... args) = 0;
> + void *obj() { return obj_; }
> + bool isObject() const { return isObject_; }
>
> protected:
> - friend class Signal<Args...>;
> void *obj_;
> + bool isObject_;
> +};
> +
> +template<typename... Args>
> +class SlotArgs : public SlotBase
> +{
> +public:
> + SlotArgs(void *obj, bool isObject)
> + : SlotBase(obj, isObject) {}
> +
> + virtual void invoke(Args... args) = 0;
> +
> +protected:
> + friend class Signal<Args...>;
> };
>
> template<typename T, typename... Args>
> -class SlotMember : public SlotBase<Args...>
> +class SlotMember : public SlotArgs<Args...>
> {
> public:
> - SlotMember(T *obj, void (T::*func)(Args...))
> - : SlotBase<Args...>(obj), func_(func) { }
> + SlotMember(T *obj, bool isObject, void (T::*func)(Args...))
> + : SlotArgs<Args...>(obj, isObject), func_(func) {}
The s/{ }/{}/ could be moved to 2/3.
>
> void invoke(Args... args) { (static_cast<T *>(this->obj_)->*func_)(args...); }
>
> @@ -45,11 +60,11 @@ private:
> };
>
> template<typename... Args>
> -class SlotStatic : public SlotBase<Args...>
> +class SlotStatic : public SlotArgs<Args...>
> {
> public:
> SlotStatic(void (*func)(Args...))
> - : SlotBase<Args...>(nullptr), func_(func) { }
> + : SlotArgs<Args...>(nullptr, false), func_(func) {}
The s/{ }/{}/ could be moved to 2/3.
>
> void invoke(Args... args) { (*func_)(args...); }
>
> @@ -58,21 +73,57 @@ private:
> void (*func_)(Args...);
> };
>
> +class SignalBase
> +{
> +public:
> + template<typename T>
> + void disconnect(T *object)
> + {
> + for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> + SlotBase *slot = *iter;
> + if (slot->obj() == object) {
> + iter = slots_.erase(iter);
> + delete slot;
> + } else {
> + ++iter;
> + }
> + }
> + }
> +
> +protected:
> + friend class Object;
> + std::list<SlotBase *> slots_;
> +};
> +
> template<typename... Args>
> -class Signal
> +class Signal : public SignalBase
> {
> public:
> Signal() { }
> ~Signal()
> {
> - for (SlotBase<Args...> *slot : slots_)
> + for (SlotBase *slot : slots_) {
> + if (slot->isObject())
> + static_cast<Object *>(slot->obj())->disconnect(this);
> delete slot;
> + }
> }
>
> +#ifndef __DOXYGEN__
> + template<typename T, typename std::enable_if<std::is_base_of<Object, T>::value>::type * = nullptr>
> + void connect(T *object, void (T::*func)(Args...))
> + {
> + object->connect(this);
> + slots_.push_back(new SlotMember<T, Args...>(object, true, func));
> + }
> +
> + template<typename T, typename std::enable_if<!std::is_base_of<Object, T>::value>::type * = nullptr>
> +#else
> template<typename T>
> +#endif
Ifdefs, templates and C++ magic what's not to love! I really like what
this enables us to do in the library and applications using it.
> void connect(T *object, void (T::*func)(Args...))
> {
> - slots_.push_back(new SlotMember<T, Args...>(object, func));
> + slots_.push_back(new SlotMember<T, Args...>(object, false, func));
> }
>
> void connect(void (*func)(Args...))
> @@ -82,7 +133,7 @@ public:
>
> void disconnect()
> {
> - for (SlotBase<Args...> *slot : slots_)
> + for (SlotBase *slot : slots_)
> delete slot;
> slots_.clear();
> }
> @@ -90,27 +141,21 @@ public:
> template<typename T>
> void disconnect(T *object)
> {
> - for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> - SlotBase<Args...> *slot = *iter;
> - if (slot->obj_ == object) {
> - iter = slots_.erase(iter);
> - delete slot;
> - } else {
> - ++iter;
> - }
> - }
> + SignalBase::disconnect(object);
> }
>
> template<typename T>
> void disconnect(T *object, void (T::*func)(Args...))
> {
> for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> - SlotBase<Args...> *slot = *iter;
> + SlotArgs<Args...> *slot = static_cast<SlotArgs<Args...> *>(*iter);
> /*
> - * If the obj_ pointer matches the object types must
> - * match, so we can safely cast to SlotMember<T, Args>.
> + * If the obj() pointer matches the object, the slot is
> + * guaranteed to be a member slot, so we can safely
> + * cast it to SlotMember<T, Args...> and access its
> + * func_ member.
> */
> - if (slot->obj_ == object &&
> + if (slot->obj() == object &&
> static_cast<SlotMember<T, Args...> *>(slot)->func_ == func) {
> iter = slots_.erase(iter);
> delete slot;
> @@ -123,8 +168,8 @@ public:
> void disconnect(void (*func)(Args...))
> {
> for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> - SlotBase<Args...> *slot = *iter;
> - if (slot->obj_ == nullptr &&
> + SlotArgs<Args...> *slot = *iter;
> + if (slot->obj() == nullptr &&
> static_cast<SlotStatic<Args...> *>(slot)->func_ == func) {
> iter = slots_.erase(iter);
> delete slot;
> @@ -140,13 +185,11 @@ public:
> * Make a copy of the slots list as the slot could call the
> * disconnect operation, invalidating the iterator.
> */
> - std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> - for (SlotBase<Args...> *slot : slots)
> - slot->invoke(args...);
> + std::vector<SlotBase *> slots{ slots_.begin(), slots_.end() };
> + for (SlotBase *slot : slots) {
> + static_cast<SlotArgs<Args...> *>(slot)->invoke(args...);
> + }
> }
> -
> -private:
> - std::list<SlotBase<Args...> *> slots_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index c5354c136563..8384cd0af451 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -10,6 +10,7 @@ libcamera_sources = files([
> 'log.cpp',
> 'media_device.cpp',
> 'media_object.cpp',
> + 'object.cpp',
> 'pipeline_handler.cpp',
> 'request.cpp',
> 'signal.cpp',
> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> new file mode 100644
> index 000000000000..826eed6f9b3a
> --- /dev/null
> +++ b/src/libcamera/object.cpp
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * object.cpp - Base object
> + */
> +
> +#include <libcamera/object.h>
> +#include <libcamera/signal.h>
> +
> +/**
> + * \file object.h
> + * \brief Base object to support automatic signal disconnection
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Object
> + * \brief Base object to support automatic signal disconnection
> + *
> + * The Object class simplifies signal/slot handling for classes implementing
> + * slots. By inheriting from Object, an object is automatically disconnected
> + * from all connected signals when it gets destroyed.
> + *
> + * \sa Signal
> + */
> +
> +Object::~Object()
> +{
> + for (SignalBase *signal : signals_)
> + signal->disconnect(this);
> +}
> +
> +void Object::connect(SignalBase *signal)
> +{
> + signals_.push_back(signal);
> +}
> +
> +void Object::disconnect(SignalBase *signal)
> +{
> + for (auto iter = signals_.begin(); iter != signals_.end(); ) {
> + if (*iter == signal)
> + iter = signals_.erase(iter);
> + else
> + iter++;
> + }
> +}
> +
> +}; /* namespace libcamera */
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> index 8d62b5beeeac..cb7daa11dab1 100644
> --- a/src/libcamera/signal.cpp
> +++ b/src/libcamera/signal.cpp
> @@ -47,6 +47,11 @@ namespace libcamera {
> * \brief Connect the signal to a member function slot
> * \param object The slot object pointer
> * \param func The slot member function
> + *
> + * If the typename T inherits from Object, the signal will be automatically
> + * disconnected from the \a func slot of \a object when \a object is destroyed.
> + * Otherwise the caller shall disconnect signals manually before destroying \a
> + * object.
> */
>
> /**
> diff --git a/test/signal.cpp b/test/signal.cpp
> index ab69ca1b663d..19a52c603a4a 100644
> --- a/test/signal.cpp
> +++ b/test/signal.cpp
> @@ -8,6 +8,7 @@
> #include <iostream>
> #include <string.h>
>
> +#include <libcamera/object.h>
> #include <libcamera/signal.h>
>
> #include "test.h"
> @@ -22,6 +23,15 @@ static void slotStatic(int value)
> valueStatic_ = value;
> }
>
> +class SlotObject : public Object
> +{
> +public:
> + void slot()
> + {
> + valueStatic_ = 1;
> + }
> +};
> +
> class SignalTest : public Test
> {
> protected:
> @@ -139,6 +149,33 @@ protected:
> return TestFail;
> }
>
> + /*
> + * Test automatic disconnection on object deletion. Connect the
> + * slot twice to ensure all instances are disconnected.
> + */
> + signalVoid_.disconnect();
> +
> + SlotObject *slotObject = new SlotObject();
> + signalVoid_.connect(slotObject, &SlotObject::slot);
> + signalVoid_.connect(slotObject, &SlotObject::slot);
> + delete slotObject;
> + valueStatic_ = 0;
> + signalVoid_.emit();
> + if (valueStatic_ != 0) {
> + cout << "Signal disconnection on object deletion test failed" << endl;
> + return TestFail;
> + }
> +
> + /*
> + * Test that signal deletion disconnects objects. This shall
> + * not generate any valgrind warning.
> + */
> + Signal<> *signal = new Signal<>();
> + slotObject = new SlotObject();
> + signal->connect(slotObject, &SlotObject::slot);
> + delete signal;
> + delete slotObject;
> +
> return TestPass;
> }
>
> --
> 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