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

[Laurent Pinchart]

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 at 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 at ideasonboard.com>

>  
>  	default:
>  		break;

-- 
Regards,

Laurent Pinchart