[libcamera-devel] [PATCH 2/3] test: message: Test recursive Thread::dispatchMessages() calls

[Jacopo Mondi]

Hi Laurent,

On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:
> The Thread::dispatchMessages() function needs to support recursive
> calls, for instance to allow flushing delivery of invoked methods. Add a
> corresponding test, which currently fails with a double free.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/test/message.cpp b/test/message.cpp
> index eeea57feab76..7895cd7c2106 100644
> --- a/test/message.cpp
> +++ b/test/message.cpp
> @@ -26,8 +26,8 @@ public:
>  		MessageReceived,
>  	};
>
> -	MessageReceiver()
> -		: status_(NoMessage)
> +	MessageReceiver(Object *parent = nullptr)
> +		: Object(parent), status_(NoMessage)
>  	{
>  	}
>
> @@ -52,6 +52,45 @@ private:
>  	Status status_;
>  };
>
> +class RecursiveMessageReceiver : public Object
> +{
> +public:
> +	RecursiveMessageReceiver()
> +		: child_(this), success_(false)
> +	{
> +	}
> +
> +	bool success() const { return success_; }
> +
> +protected:
> +	void message([[maybe_unused]] Message *msg)
> +	{
> +		if (msg->type() != Message::None) {
> +			Object::message(msg);
> +			return;
> +		}
> +
> +		child_.postMessage(std::make_unique<Message>(Message::None));
> +
> +		/*
> +		 * If the child has already received the message, something is
> +		 * wrong.
> +		 */
> +		if (child_.status() != MessageReceiver::NoMessage)
> +			return;
> +
> +		Thread::current()->dispatchMessages(Message::None);
> +
> +		/* The child should now have received the message. */
> +		if (child_.status() == MessageReceiver::MessageReceived)
> +			success_ = true;
> +	}
> +
> +private:
> +	MessageReceiver child_;
> +	bool success_;
> +};
> +
>  class SlowMessageReceiver : public Object
>  {
>  protected:
> @@ -120,6 +159,29 @@ protected:
>
>  		delete slowReceiver;
>
> +		this_thread::sleep_for(chrono::milliseconds(100));
> +
> +		/*
> +		 * Test recursive calls to Thread::dispatchMessages(). Messages
> +		 * should be delivered correctly, without crashes or memory
> +		 * leaks. Two messages need to be posted to ensure we don't only
> +		 * test the simple case of a queue containing a single message.
> +		 */
> +		RecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();
> +		recursiveReceiver->moveToThread(&thread_);
> +
> +		recursiveReceiver->postMessage(std::make_unique<Message>(Message::None));
> +		recursiveReceiver->postMessage(std::make_unique<Message>(Message::UserMessage));

I'm not sure why two messages are necessary, also considering that
messages != Message::None are ignored...

If my understanding is correct, a single postMessage() triggers a
dispatcher->interrupt() which triggers a dispatchMessages() as the receiver
runs on a different thread and can be interrupted. The message is then
dispatched and handled by the execution of RecursiveMessageReceiver::message()
call.

>From there we post a message to the child and go throuh the same path if not
that the child runs on the same thread and we have to dispatch
messages forcefully, causing a recursive call to dispatchMessages() on
the same thread of execution.

Am I missing something or is the second message not required ?

Thanks
  j

> +
> +		this_thread::sleep_for(chrono::milliseconds(10));
> +
> +		if (!recursiveReceiver->success()) {
> +			cout << "Recursive message delivery failed" << endl;
> +			return TestFail;
> +		}
> +
> +		delete recursiveReceiver;
> +
>  		return TestPass;
>  	}
>
> --
> Regards,
>
> Laurent Pinchart
>