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

9042 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
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>
---
 include/libcamera/internal/message.h |  1 +
 include/libcamera/object.h           |  2 ++
 src/libcamera/object.cpp             | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+)

Hi Umang,

Thank you for the patch.

On Tue, Jul 28, 2020 at 10:57:24AM +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

s/thread,/thread/

> threading model.

s/threading model/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>
> ---
>  include/libcamera/internal/message.h |  1 +
>  include/libcamera/object.h           |  2 ++
>  src/libcamera/object.cpp             | 20 ++++++++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/include/libcamera/internal/message.h b/include/libcamera/internal/message.h
> index 92ea64a..f1b133b 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,

This needs to be documented in message.cpp. Just "Object is scheduled
for deletion" should be enough.

>  		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/object.cpp b/src/libcamera/object.cpp
> index 1544a23..93e683a 100644
> --- a/src/libcamera/object.cpp
> +++ b/src/libcamera/object.cpp
> @@ -98,6 +98,22 @@ Object::~Object()
>  		child->parent_ = nullptr;
>  }
>  
> +/**
> + * \brief Defer the deletion of Object instance to their thread

Being a bit pendantic, the function doesn't defer the deletion of the
object (as there's no pending deletion before it's called), but
schedules a deferred deletion. Maybe "Schedule deletion of the instance
in the thread it belongs to" ?

> + *
> + * Schedule deletion of the Object in the thread to which they are bound.
> + * This will prevent destroying the Object from a different thread, which is
> + * not allowed by the threading model.

I think we should mention the event loop here to give a bit more
information about when the object will be deleted exactly. Maybe

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

I think we also have an issue if the object belongs to a thread that
doesn't have an event loop. We can fix that later, just adding

 * \todo Handle scheduled deletion when the thread doesn't have an event loop

for now.

> + *
> + * It will typically be used with a custom deleter when creating a shared
> + * pointer. It can then be ensured that the Object is deleted in its
> + * own thread even if its last reference is dropped in a different thread.

Even though this is our first use case, I'm not sure it would be the
most typical one. Maybe we can mention it as just an example instead ?

 * 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<MyObjet> 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

And you should add

 * \context This function is \threadsafe.

> + */

While at it, should we add

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

to the Object::~Object() documentation ?

> +void Object::deleteLater()
> +{
> +	postMessage(std::make_unique<Message>(Message::DeferredDelete));
> +}
> +
>  /**
>   * \brief Post a message to the object's thread
>   * \param[in] msg The message
> @@ -143,6 +159,10 @@ void Object::message(Message *msg)
>  
>  		break;
>  	}

A blank line here would be nice.

> +	case Message::DeferredDelete: {
> +		delete this;
> +		break;
> +	}

No need for the curly braces.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	default:
>  		break;