| Message ID | 20191127084909.10612-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 7df177fd889624c4b149ba2ecabb4969ad8c2bee |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-11-27 10:49:08 +0200, 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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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. > + * > * 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, 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@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. > * 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. >
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@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.
Hi Laurent, On 27/11/2019 15:20, Laurent Pinchart wrote: > 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@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? -- Kieran > >>> * 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. >
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@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.
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. + * * 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.
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@ideasonboard.com> --- src/libcamera/object.cpp | 4 ++++ 1 file changed, 4 insertions(+)