[libcamera-devel] [PATCH 4/5] libcamera: object: Document danger of deleting object from other thread

[Laurent Pinchart]

Hi Kieran,

On Wed, Nov 27, 2019 at 03:42:29PM +0000, Kieran Bingham wrote:
> On 27/11/2019 15:20, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2019 at 03:14:08PM +0000, Kieran Bingham wrote:
> >> On 27/11/2019 08:49, Laurent Pinchart wrote:
> >>> Object instances receive messages dispatched from the event loop of the
> >>> thread they belong to. Deleting an object from a different thread is
> >>> thus dangerous, unless the caller ensures that no message delivery is in
> >>> progress. Document this in the Object class documentation.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>  src/libcamera/object.cpp | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp
> >>> index 1f787271f782..e76faf48b8ed 100644
> >>> --- a/src/libcamera/object.cpp
> >>> +++ b/src/libcamera/object.cpp
> >>> @@ -40,6 +40,10 @@ LOG_DEFINE_CATEGORY(Object)
> >>>   * implementing easy message passing between threads by inheriting from the
> >>>   * Object class.
> >>>   *
> >>> + * Deleting an object from a thread other than the one the object is bound to is
> >>> + * unsafe, unless the caller ensures that the object isn't processing any
> >>> + * message concurrently.
> >>> + *
> >>
> >> What about codifying this with an assertion or Log(Error) check?
> >>
> >> I.e. put something into the destructor which tests the current thread
> >> against the object bound thread?
> >>
> >> I think a runtime check here would be cheap, and serve a better way to
> >> catch these bugs than simply documenting potential issues.
> > 
> > I'd do so if it wasn't for the "unless the caller" part. It's valid to
> > destroy objects from other threads if we're careful, and one simple use
> > case is destroying objects that have been moved to a thread after the
> > thread finishes.
> 
> But the message says <paraphrasing>
>  "From an object if the object is processing a message"
> 
> So doesn't that make the check (psuedo-code):
> 
>  if (pendingMessages_ && this_thread != object->bound_thread())
> 	Log(Error)
> 	  << "Destroying object from separate thread with messages";
> 
> 
> Or maybe I've misunderstood something?

The issue isn't pendingMessages_, that's handled by calling
Thread::removeMessages() from Object::~Object(). The issue is the
object's message() method being called from Thread::dispatchMessages()
and the call being in progress when the object is destroyed from another
thread.

> >>>   * Object slots connected to signals will also run in the context of the
> >>>   * object's thread, regardless of whether the signal is emitted in the same or
> >>>   * in another thread.

-- 
Regards,

Laurent Pinchart