[libcamera-devel,v3,2/3] libcamera: object: Add deleteLater() support

9089 diff mbox series

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@uajain.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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

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@uajain.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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)

Hi Umang,

Looks like I forgot to review the test case.

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@uajain.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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,
>  		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));
> +}
> +
>  /**
>   * \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@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;

This should be static, and named objectDeleted (all variables start with
a lower case).

> +
> +class NewThread : public Thread
> +{
> +public:
> +	NewThread(Object *obj = nullptr)
> +		: object_(obj), deleteScheduled_(false)
> +	{
> +	}
> +
> +	bool objectScheduledForDeletion() { return deleteScheduled_; }
> +
> +protected:
> +        void run()
> +        {
> +		object_->deleteLater();
> +		deleteScheduled_ = true;
> +        }
> +
> +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;

This can be a local variable in the run() function.

> +
> +		return 0;
> +	}
> +
> +	int run()
> +	{
> +		TestObject *obj;
> +
> +		obj = new TestObject();

You can combine these two lines.

> +		NewThread thread(obj);
> +		thread.start();
> +		Timer timer;
> +		timer.start(100);
> +
> +		while (timer.isRunning() && !isDeleteScheduled_)
> +			isDeleteScheduled_ = thread.objectScheduledForDeletion();
> +
> +		thread.exit();

This isn't needed, as the thread doesn't run any event loop (the exec()
function isn't called by NewThread::run()).

> +		thread.wait();

I think you can simplify this by dropping the timer and the
isDeleteScheduled_ variable, we can assume the thread will run (there
are specific tests for that), so just

	NewThread thread(obj);
	thread.start();
	thread.wait();

should be enough.

> +
> +		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;

Maybe "Failed to dispatch DeferredDelete" ?

> +			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.
> +		 */

How about just

		obj = new TestObject();
		obj->deleteLater();
		obj->deleteLater();

and turning ObjectDeleted into objectDeleteCount, and verifying that the
counter is equal to 1 ?

I've ended up doing this while investigating another issue (which turned
out not to be an issue), so I'll send a patch for the test case shortly.
I propose merging this patch without the test case, and keeping the test
in a separate patch (I've retained your authorship of course).

> +		Thread::current()->dispatchMessages(Message::Type::DeferredDelete);
> +		if(!ObjectDeleted) {
> +			cout << "Failed to delete object";
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +	}
> +
> +private:
> +	bool isDeleteScheduled_;
> +};
> +
> +TEST_REGISTER(ObjectDeleteTest)