[libcamera-devel] [PATCH v3 2/3] libcamera: object: Add deleteLater() support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 31 16:28:06 CEST 2020
Hi Umang,
Thank you for the patch.
On Fri, Jul 31, 2020 at 01:39:55PM +0000, Umang Jain wrote:
> This commit adds support to schedule the deletion of an Object to the
> thread it is bound to (similar to [1]). An Object getting destroyed
> by a different thread is considered as a violation as per the
> libcamera threading model.
>
> This will be useful for an Object where its ownership is shared via
> shared pointers in different threads. If the thread which drops the
> last reference of the Object is a different thread, the destructors
> get called in that particular thread, not the one Object is bound to.
> Hence, in order to resolve this kind of situation, the creation of
> shared pointer can be accompanied by a custom deleter which in turns
> use deleteLater() to ensure the Object is destroyed in its own thread.
>
> [1] https://doc.qt.io/qt-5/qobject.html#deleteLater
>
> Signed-off-by: Umang Jain <email at uajain.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/message.h | 1 +
> include/libcamera/object.h | 2 +
> src/libcamera/message.cpp | 2 +
> src/libcamera/object.cpp | 48 ++++++++++
> test/meson.build | 1 +
> test/object-delete.cpp | 125 +++++++++++++++++++++++++++
> 6 files changed, 179 insertions(+)
> create mode 100644 test/object-delete.cpp
>
> diff --git a/include/libcamera/internal/message.h b/include/libcamera/internal/message.h
> index 92ea64a..98e5a28 100644
> --- a/include/libcamera/internal/message.h
> +++ b/include/libcamera/internal/message.h
> @@ -25,6 +25,7 @@ public:
> None = 0,
> InvokeMessage = 1,
> ThreadMoveMessage = 2,
> + DeferredDelete=3,
Missing spaces.
> UserMessage = 1000,
> };
>
> diff --git a/include/libcamera/object.h b/include/libcamera/object.h
> index 9a3dd07..a1882f0 100644
> --- a/include/libcamera/object.h
> +++ b/include/libcamera/object.h
> @@ -27,6 +27,8 @@ public:
> Object(Object *parent = nullptr);
> virtual ~Object();
>
> + void deleteLater();
> +
> void postMessage(std::unique_ptr<Message> msg);
>
> template<typename T, typename R, typename... FuncArgs, typename... Args,
> diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp
> index e9b3e73..bc985c0 100644
> --- a/src/libcamera/message.cpp
> +++ b/src/libcamera/message.cpp
> @@ -49,6 +49,8 @@ std::atomic_uint Message::nextUserType_{ Message::UserMessage };
> * \brief Asynchronous method invocation across threads
> * \var Message::ThreadMoveMessage
> * \brief Object is being moved to a different thread
> + * \var Message::DeferredDelete
> + * \brief Object is scheduled for deletion
> * \var Message::UserMessage
> * \brief First value available for user-defined messages
> */
> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> index 1544a23..74e2268 100644
> --- a/src/libcamera/object.cpp
> +++ b/src/libcamera/object.cpp
> @@ -73,6 +73,10 @@ Object::Object(Object *parent)
> * Deleting an Object automatically disconnects all signals from the Object's
> * slots. All the Object's children are made orphan, but stay bound to their
> * current thread.
> + *
> + * Object instances shall be destroyed from the thread they are bound to,
> + * otherwise undefined behaviour may occur. If deletion of an Object needs to
> + * be scheduled from a different thread, deleteLater() shall be used.
> */
> Object::~Object()
> {
> @@ -98,6 +102,46 @@ Object::~Object()
> child->parent_ = nullptr;
> }
>
> +/**
> + * \brief Schedule deletion of the instance in the thread it belongs to
> + *
> + * This function schedules deletion of the Object when control returns to the
> + * event loop that the object belongs to. This ensures the object is destroyed
> + * from the right context, as required by the libcamera threading model.
> + *
> + * If this function is called before the thread's event loop is started, the
> + * object will be deleted when the event loop starts.
> + *
> + * Deferred deletion can be used to control the destruction context with shared
> + * pointers. An object managed with shared pointers is deleted when the last
> + * reference is destroyed, which makes difficult to ensure through software
> + * design which context the deletion will take place in. With a custom deleter
> + * for the shared pointer using deleteLater(), the deletion can be guaranteed to
> + * happen in the thread the object is bound to.
> + *
> + * \code{.cpp}
> + * std::shared_ptr<MyObject> createObject()
> + * {
> + * struct Deleter : std::default_delete<MyObject> {
> + * void operator()(MyObject *obj)
> + * {
> + * delete obj;
> + * }
> + * };
> + *
> + * MyObject *obj = new MyObject();
> + *
> + * return std::shared_ptr<MyObject>(obj, Deleter());
> + * }
> + * \endcode
> + *
> + * \context This function is \threadsafe.
> + */
> +void Object::deleteLater()
> +{
> + postMessage(std::make_unique<Message>(Message::DeferredDelete));
This uses spaces instead of tabs for indentation.
> +}
> +
> /**
> * \brief Post a message to the object's thread
> * \param[in] msg The message
> @@ -144,6 +188,10 @@ void Object::message(Message *msg)
> break;
> }
>
> + case Message::DeferredDelete:
> + delete this;
> + break;
> +
> default:
> break;
> }
> diff --git a/test/meson.build b/test/meson.build
> index f41d6e7..f5aa144 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -34,6 +34,7 @@ internal_tests = [
> ['hotplug-cameras', 'hotplug-cameras.cpp'],
> ['message', 'message.cpp'],
> ['object', 'object.cpp'],
> + ['object-delete', 'object-delete.cpp'],
> ['object-invoke', 'object-invoke.cpp'],
> ['pixel-format', 'pixel-format.cpp'],
> ['signal-threads', 'signal-threads.cpp'],
> diff --git a/test/object-delete.cpp b/test/object-delete.cpp
> new file mode 100644
> index 0000000..6cd1a1e
> --- /dev/null
> +++ b/test/object-delete.cpp
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Umang Jain <email at uajain.com>
> + *
> + * object.cpp - Object deletion tests
> + */
> +
> +#include <iostream>
> +
> +#include <libcamera/object.h>
> +#include <libcamera/timer.h>
> +
> +#include "libcamera/internal/thread.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/*
> + * Global flag to check if object is on its way to deletion from it's
> + * own thread only.
> + */
> +bool ObjectDeleted = false;
> +
> +class NewThread : public Thread
> +{
> +public:
> + NewThread(Object *obj = nullptr)
> + : object_(obj), deleteScheduled_(false)
> + {
> + }
> +
> + bool objectScheduledForDeletion() { return deleteScheduled_; }
> +
> +protected:
> + void run()
> + {
> + object_->deleteLater();
> + deleteScheduled_ = true;
> + }
This uses spaces too.
> +
> +private:
> + Object *object_;
> + bool deleteScheduled_;
> +};
> +
> +
> +class TestObject : public Object
> +{
> +public:
> + TestObject()
> + {
> + }
> +
> + ~TestObject()
> + {
> + if (thread() == Thread::current())
> + ObjectDeleted = true;
> + }
> +};
> +
> +class ObjectDeleteTest : public Test
> +{
> +protected:
> + int init()
> + {
> + isDeleteScheduled_ = false;
> +
> + return 0;
> + }
> +
> + int run()
> + {
> + TestObject *obj;
> +
> + obj = new TestObject();
> + NewThread thread(obj);
> + thread.start();
> + Timer timer;
> + timer.start(100);
> +
> + while (timer.isRunning() && !isDeleteScheduled_)
> + isDeleteScheduled_ = thread.objectScheduledForDeletion();
> +
> + thread.exit();
> + thread.wait();
> +
> + if (!isDeleteScheduled_) {
> + cout << "Failed to queue object for deletion from another thread" << endl;
> + return TestFail;
> + }
> +
> + Thread::current()->dispatchMessages(Message::Type::DeferredDelete);
> +
> + if (!ObjectDeleted) {
> + cout << "Failed to delete object queued from another thread" << endl;
> + return TestFail;
> + }
> +
> + ObjectDeleted = false;
> + obj = new TestObject();
> + obj->deleteLater();
> + /*
> + * \todo Devise a check for multiple deleteLater() calls to a object.
> + * It should be checked that the object is deleted only once.
> + */
> + Thread::current()->dispatchMessages(Message::Type::DeferredDelete);
> + if(!ObjectDeleted) {
Missing space.
Didn't checkstyle.py warn you ?
I'll fix all this when applying.
> + cout << "Failed to delete object";
> + return TestFail;
> + }
> +
> + return TestPass;
> + }
> +
> + void cleanup()
> + {
> + }
> +
> +private:
> + bool isDeleteScheduled_;
> +};
> +
> +TEST_REGISTER(ObjectDeleteTest)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list