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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran