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

[Laurent Pinchart]

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.

> > 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();

-- 
Regards,

Laurent Pinchart