[v2,04/12] libcamera: thread: Ensure deferred deletion of all objects before stopping

19452 diff mbox series

Objects can be scheduled for deletion with Object::deleteLater(), which
queues a deferred deletion to the thread's event loop. As the
deleteLater() function is meant to be called from a different thread,
this may race with thread termination, and deferred deletions queued
just before calling Thread::exit() may not be processed by the event
loop. Make sure they get processed when finishing the thread, before
stopping.

This eliminates the race condition that occurs when calling
Object::deleteLater() followed by Thread::exit() from the same thread.
Calling deleteLater() from neither the thread the object is bound to or
the thread calling Thread::exit() is still inherently racy.

The change fixes a failure in the object-delete unit test.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Expand commit message
- Update the Object::deleteLater() documentation
---
 src/libcamera/base/object.cpp | 5 +++--
 src/libcamera/base/thread.cpp | 6 ++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Objects can be scheduled for deletion with Object::deleteLater(), which
> queues a deferred deletion to the thread's event loop. As the
> deleteLater() function is meant to be called from a different thread,
> this may race with thread termination, and deferred deletions queued
> just before calling Thread::exit() may not be processed by the event
> loop. Make sure they get processed when finishing the thread, before
> stopping.
>
> This eliminates the race condition that occurs when calling
> Object::deleteLater() followed by Thread::exit() from the same thread.
> Calling deleteLater() from neither the thread the object is bound to or
> the thread calling Thread::exit() is still inherently racy.
>
> The change fixes a failure in the object-delete unit test.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
> Changes since v1:
>
> - Expand commit message
> - Update the Object::deleteLater() documentation
> ---
>  src/libcamera/base/object.cpp | 5 +++--
>  src/libcamera/base/thread.cpp | 6 ++++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp
> index 1fce5a2af9af..14399d750e03 100644
> --- a/src/libcamera/base/object.cpp
> +++ b/src/libcamera/base/object.cpp
> @@ -116,8 +116,9 @@ Object::~Object()
>   * 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.
> + * If this function is called before the thread's event loop is started or after
> + * it has stopped, the object will be deleted when the event loop (re)starts. If
> + * this never occurs, the object will be leaked.
>   *
>   * Deferred deletion can be used to control the destruction context with shared
>   * pointers. An object managed with shared pointers is deleted when the last
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 75693c92a0b1..4ac72036aa69 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -371,6 +371,12 @@ void Thread::run()
>  
>  void Thread::finishThread()
>  {
> +	/*
> +	 * Objects may have been scheduled for deletion right before the thread
> +	 * exited. Ensure they get deleted now, before the thread stops.
> +	 */
> +	dispatchMessages(Message::Type::DeferredDelete);
> +
>  	data_->mutex_.lock();
>  	data_->running_ = false;
>  	data_->mutex_.unlock();