[libcamera-devel] [PATCH] libcamera: base: thread: Prevent messages to thread without an event loop

[Umang Jain]

Hi,

On 10/7/22 3:12 PM, Umang Jain via libcamera-devel wrote:
> Hi Laurent,
>
> Thank you for the explanation.
>
> Need few clarifications below
>
> On 6/28/22 3:16 PM, Laurent Pinchart via libcamera-devel wrote:
>> On Tue, Jun 28, 2022 at 11:09:21AM +0300, Tomi Valkeinen wrote:
>>> On 27/06/2022 19:46, Laurent Pinchart wrote:
>>>> A message posted to an object bound to a thread without an event loop
>>>> will never get delivered. This indicates a bug in the caller, and
>>>> materializes in ways that can be hard to debug, such as a signal never
>>>> being delivered. Add an assertion in Thread::postMessage() to catch 
>>>> this
>>>> at the source.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>> ---
>>>>    src/libcamera/base/thread.cpp | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/src/libcamera/base/thread.cpp 
>>>> b/src/libcamera/base/thread.cpp
>>>> index 6bda9d1462f5..6ead1d7c12d0 100644
>>>> --- a/src/libcamera/base/thread.cpp
>>>> +++ b/src/libcamera/base/thread.cpp
>>>> @@ -530,6 +530,7 @@ void 
>>>> Thread::postMessage(std::unique_ptr<Message> msg, Object *receiver)
>>>>        msg->receiver_ = receiver;
>>>>           ASSERT(data_ == receiver->thread()->data_);
>>>> +    ASSERT(data_ != mainThread.data_);
>
> Simply reading the commit message and inferring, the first change I 
> expected was on the lines;
>         ASSERT (data_->dispatcher_ != nullptr)
>
> or for readability,
>
>       ASSERT(receiver->thread()->data_->dispatcher_ != nullptr);
>
>
> that is, checking the receiver's thread event loop exists or not.
>>>>           MutexLocker locker(data_->messages_.mutex_);
>>>>        data_->messages_.list_.push_back(std::move(msg));
>>>>
>>>> base-commit: 27cc0a6b58bcca32071cb6ab96e5ee79c75031e5
>>> So what was the issue with the code I had? The problem is that
>>> CameraManager is a libcamera Object, and was associated with the main
>>> thread...? And a PyCameraManager which doesn't inherit CameraManager is
>>> not an Object and not associated with any thread, so it works?
>> That's right. Because PyCameraManager inherited from Object, it
>> automatically got support for queued signal delivery, where a signal
>> emitted in one thread would be processed in a different thread. This
>> mechanism requires the receiving object to be bound to a thread that has
>> an event loop managed by libcamera. By default an Object is bound to the
>> thread it's created in.
>>
>> The CameraManager is created in the Python main thread, and the signal
>> is emitted from an internal libcamera thread. The signal will thus be
>> queued (the default connection type is Auto, which means that signals
>> are delivered directly if the receiver object is bound to the thread
>> that the signal is emitted from, or queued otherwise), which queues a
>> message for the receiver's thread event loop. As the receiver thread is
>> the Python main thread (the application thread from a libcamera point of
>> view), which doesn't have a libcamera event loop, the message is never
>> processed.
>
> I understand the explanation above.
>
> However I'm a bit lost with mainThread static variable. Am I right to 
> infer that the mainThread is the Python's main thread in this case? If 
> yes, the patch is correct, refraining the messages to be posted to a 
> object whose threads has no event loop.
>
> However, how about if the mainThread inherits from libcamera (hence 
> Object, Thread, Signal delivery is in place) but tries to send a 
> message to another Object bound to itself (i.e. mainThread)
> Won't the assertion in this patch fail ?

Ran the test suite with this patch and saw 3 failures

66/71 libcamera / timer-thread FAIL           15.52s   killed by signal 
6 SIGABRT
67/71 libcamera / event-thread FAIL           23.13s   killed by signal 
6 SIGABRT
68/71 libcamera / object-invoke FAIL           23.99s   killed by signal 
6 SIGABRT
69/71 libcamera / object-delete TIMEOUT        30.02s   killed by signal 
15 SIGTERM

All triggering the assertion.
>
>> If the receiver doesn't inherit from the Object class, then the signal
>> will be delivered directly, in the emitter's thread. You can achieve the
>> same result with an Object receiver by specifying the direct connection
>> type when connecting the signal.
>>
>