[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: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.

> >   * 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