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

[Milan Zamazal]

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

> On Mon, Jan 22, 2024 at 08:44:39PM +0100, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart at 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.
>> 
>> Does this eliminate the race completely or does it just reduce it significantly?
>> This should be mentioned and if a possible race still exists, it would be good
>> to mention the practical benefits of the change (like making tests reliable).
>
> It eliminates this particular race completely. I'll clarify this with
>
> 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.
>
>> deleteLater() documentation is not clear (on purpose?) about whether the
>> deletion is guaranteed to be run.  Maybe worth to clarify it there.
>
> I'll improve the Object::deleteLater() documentation as follows:
>
> - * 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.

Thanks, these explanations confirm what I expected and are completely clear now,
at least to me.

>> > This fixes a failure in the object-delete unit test.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> > ---
>> >  src/libcamera/base/thread.cpp | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > 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();