| Message ID | 20240121035948.4226-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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. 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). deleteLater() documentation is not clear (on purpose?) about whether the deletion is guaranteed to be run. Maybe worth to clarify it there. > This fixes a failure in the object-delete unit test. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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();
On Mon, Jan 22, 2024 at 08:44:39PM +0100, Milan Zamazal wrote: > 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. > > 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@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();
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Mon, Jan 22, 2024 at 08:44:39PM +0100, Milan Zamazal wrote: >> 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. >> >> 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@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();
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();
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 fixes a failure in the object-delete unit test. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/base/thread.cpp | 6 ++++++ 1 file changed, 6 insertions(+)