[{"id":17976,"web_url":"https://patchwork.libcamera.org/comment/17976/","msgid":"<20210705110829.ligzn7qhla67v77g@uno.localdomain>","date":"2021-07-05T11:08:29","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n> The Thread::dispatchMessages() function needs to support recursive\n> calls, for instance to allow flushing delivery of invoked methods. Add a\n> corresponding test, which currently fails with a double free.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 64 insertions(+), 2 deletions(-)\n>\n> diff --git a/test/message.cpp b/test/message.cpp\n> index eeea57feab76..7895cd7c2106 100644\n> --- a/test/message.cpp\n> +++ b/test/message.cpp\n> @@ -26,8 +26,8 @@ public:\n> \t\tMessageReceived,\n> \t};\n>\n> -\tMessageReceiver()\n> -\t\t: status_(NoMessage)\n> +\tMessageReceiver(Object *parent = nullptr)\n> +\t\t: Object(parent), status_(NoMessage)\n> \t{\n> \t}\n>\n> @@ -52,6 +52,45 @@ private:\n> \tStatus status_;\n> };\n>\n> +class RecursiveMessageReceiver : public Object\n> +{\n> +public:\n> +\tRecursiveMessageReceiver()\n> +\t\t: child_(this), success_(false)\n> +\t{\n> +\t}\n> +\n> +\tbool success() const { return success_; }\n> +\n> +protected:\n> +\tvoid message([[maybe_unused]] Message *msg)\n> +\t{\n> +\t\tif (msg->type() != Message::None) {\n> +\t\t\tObject::message(msg);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tchild_.postMessage(std::make_unique(Message::None));\n> +\n> +\t\t/*\n> +\t\t * If the child has already received the message, something is\n> +\t\t * wrong.\n> +\t\t */\n> +\t\tif (child_.status() != MessageReceiver::NoMessage)\n> +\t\t\treturn;\n> +\n> +\t\tThread::current()->dispatchMessages(Message::None);\n> +\n> +\t\t/* The child should now have received the message. */\n> +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n> +\t\t\tsuccess_ = true;\n> +\t}\n> +\n> +private:\n> +\tMessageReceiver child_;\n> +\tbool success_;\n> +};\n> +\n> class SlowMessageReceiver : public Object\n> {\n> protected:\n> @@ -120,6 +159,29 @@ protected:\n>\n> \t\tdelete slowReceiver;\n>\n> +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> +\n> +\t\t/*\n> +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n> +\t\t * should be delivered correctly, without crashes or memory\n> +\t\t * leaks. Two messages need to be posted to ensure we don't only\n> +\t\t * test the simple case of a queue containing a single message.\n> +\t\t */\n> +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n> +\t\trecursiveReceiver->moveToThread(&thread_);\n> +\n> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n\nI'm not sure why two messages are necessary, also considering that\nmessages != Message::None are ignored...\n\nIf my understanding is correct, a single postMessage() triggers a\ndispatcher->interrupt() which triggers a dispatchMessages() as the receiver\nruns on a different thread and can be interrupted. The message is then\ndispatched and handled by the execution of RecursiveMessageReceiver::message()\ncall.\n\nFrom there we post a message to the child and go throuh the same path if not\nthat the child runs on the same thread and we have to dispatch\nmessages forcefully, causing a recursive call to dispatchMessages() on\nthe same thread of execution.\n\nAm I missing something or is the second message not required ?\n\nThanks\n j\n\n> +\n> +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n> +\n> +\t\tif (!recursiveReceiver->success()) {\n> +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tdelete recursiveReceiver;\n> +\n> \t\treturn TestPass;\n> \t}\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2DC2ABD794\n\tfor ;\n\tMon, 5 Jul 2021 11:07:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 585AF684E4;\n\tMon, 5 Jul 2021 13:07:42 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D77A66050A\n\tfor ;\n\tMon, 5 Jul 2021 13:07:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6645C1BF207;\n\tMon, 5 Jul 2021 11:07:40 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 13:08:29 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705110829.ligzn7qhla67v77g@uno.localdomain>","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17980,"web_url":"https://patchwork.libcamera.org/comment/17980/","msgid":"","date":"2021-07-05T12:16:32","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n> > The Thread::dispatchMessages() function needs to support recursive\n> > calls, for instance to allow flushing delivery of invoked methods. Add a\n> > corresponding test, which currently fails with a double free.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n> > 1 file changed, 64 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/test/message.cpp b/test/message.cpp\n> > index eeea57feab76..7895cd7c2106 100644\n> > --- a/test/message.cpp\n> > +++ b/test/message.cpp\n> > @@ -26,8 +26,8 @@ public:\n> > \t\tMessageReceived,\n> > \t};\n> >\n> > -\tMessageReceiver()\n> > -\t\t: status_(NoMessage)\n> > +\tMessageReceiver(Object *parent = nullptr)\n> > +\t\t: Object(parent), status_(NoMessage)\n> > \t{\n> > \t}\n> >\n> > @@ -52,6 +52,45 @@ private:\n> > \tStatus status_;\n> > };\n> >\n> > +class RecursiveMessageReceiver : public Object\n> > +{\n> > +public:\n> > +\tRecursiveMessageReceiver()\n> > +\t\t: child_(this), success_(false)\n> > +\t{\n> > +\t}\n> > +\n> > +\tbool success() const { return success_; }\n> > +\n> > +protected:\n> > +\tvoid message([[maybe_unused]] Message *msg)\n> > +\t{\n> > +\t\tif (msg->type() != Message::None) {\n> > +\t\t\tObject::message(msg);\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tchild_.postMessage(std::make_unique(Message::None));\n> > +\n> > +\t\t/*\n> > +\t\t * If the child has already received the message, something is\n> > +\t\t * wrong.\n> > +\t\t */\n> > +\t\tif (child_.status() != MessageReceiver::NoMessage)\n> > +\t\t\treturn;\n> > +\n> > +\t\tThread::current()->dispatchMessages(Message::None);\n> > +\n> > +\t\t/* The child should now have received the message. */\n> > +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n> > +\t\t\tsuccess_ = true;\n> > +\t}\n> > +\n> > +private:\n> > +\tMessageReceiver child_;\n> > +\tbool success_;\n> > +};\n> > +\n> > class SlowMessageReceiver : public Object\n> > {\n> > protected:\n> > @@ -120,6 +159,29 @@ protected:\n> >\n> > \t\tdelete slowReceiver;\n> >\n> > +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> > +\n> > +\t\t/*\n> > +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n> > +\t\t * should be delivered correctly, without crashes or memory\n> > +\t\t * leaks. Two messages need to be posted to ensure we don't only\n> > +\t\t * test the simple case of a queue containing a single message.\n> > +\t\t */\n> > +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n> > +\t\trecursiveReceiver->moveToThread(&thread_);\n> > +\n> > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n> > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n> \n> I'm not sure why two messages are necessary, also considering that\n> messages != Message::None are ignored...\n> \n> If my understanding is correct, a single postMessage() triggers a\n> dispatcher->interrupt() which triggers a dispatchMessages() as the receiver\n> runs on a different thread and can be interrupted. The message is then\n> dispatched and handled by the execution of RecursiveMessageReceiver::message()\n> call.\n> \n> From there we post a message to the child and go throuh the same path if not\n> that the child runs on the same thread and we have to dispatch\n> messages forcefully, causing a recursive call to dispatchMessages() on\n> the same thread of execution.\n> \n> Am I missing something or is the second message not required ?\n\nThe second message is needed to ensure we don't only test a case where\nthe queue has a single message. The test doesn't fail in that case, due\nto the fact that the internal iter variable in\nThread::dispatchMessages() is incremented before dispatching the current\nmessage. If the list contains a single message, iter will thus be the\nend() of the list after being incremented, which will not cause the\nouter dispatchMessage() call to fail with use-after-free (the .end()\niterator is not invalidated by an erase operation on a std::list).\n\n> > +\n> > +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n> > +\n> > +\t\tif (!recursiveReceiver->success()) {\n> > +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tdelete recursiveReceiver;\n> > +\n> > \t\treturn TestPass;\n> > \t}\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97EE3C0100\n\tfor ;\n\tMon, 5 Jul 2021 12:17:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01D1868501;\n\tMon, 5 Jul 2021 14:17:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 601F26050A\n\tfor ;\n\tMon, 5 Jul 2021 14:17:14 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1D49880;\n\tMon, 5 Jul 2021 14:17:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dokwkvqs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625487433;\n\tbh=FCN3IKB3TWyggqEbe7I1XCEb3WIimSVvzwaFGlrsdeo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dokwkvqsdw/NVsgKzdJvb1+Ro7OnjQJ06L5I2SEo2rV5K0IM2W/DHAqwRnZeB22vO\n\taBJ6qa8hU8ywZmfPW/PyohckrL8nJo7lNBx0FzWp+SCQwmnaOje8AXnGRIEXyxM5l8\n\tpBzOiysRTJaM5a3LpIg/kjspoLLGqipwmYZOh49M=","Date":"Mon, 5 Jul 2021 15:16:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>\n\t<20210705110829.ligzn7qhla67v77g@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210705110829.ligzn7qhla67v77g@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17982,"web_url":"https://patchwork.libcamera.org/comment/17982/","msgid":"<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>","date":"2021-07-05T12:38:11","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n> > > The Thread::dispatchMessages() function needs to support recursive\n> > > calls, for instance to allow flushing delivery of invoked methods. Add a\n> > > corresponding test, which currently fails with a double free.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n> > > 1 file changed, 64 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/test/message.cpp b/test/message.cpp\n> > > index eeea57feab76..7895cd7c2106 100644\n> > > --- a/test/message.cpp\n> > > +++ b/test/message.cpp\n> > > @@ -26,8 +26,8 @@ public:\n> > > \t\tMessageReceived,\n> > > \t};\n> > >\n> > > -\tMessageReceiver()\n> > > -\t\t: status_(NoMessage)\n> > > +\tMessageReceiver(Object *parent = nullptr)\n> > > +\t\t: Object(parent), status_(NoMessage)\n> > > \t{\n> > > \t}\n> > >\n> > > @@ -52,6 +52,45 @@ private:\n> > > \tStatus status_;\n> > > };\n> > >\n> > > +class RecursiveMessageReceiver : public Object\n> > > +{\n> > > +public:\n> > > +\tRecursiveMessageReceiver()\n> > > +\t\t: child_(this), success_(false)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tbool success() const { return success_; }\n> > > +\n> > > +protected:\n> > > +\tvoid message([[maybe_unused]] Message *msg)\n> > > +\t{\n> > > +\t\tif (msg->type() != Message::None) {\n> > > +\t\t\tObject::message(msg);\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tchild_.postMessage(std::make_unique(Message::None));\n> > > +\n> > > +\t\t/*\n> > > +\t\t * If the child has already received the message, something is\n> > > +\t\t * wrong.\n> > > +\t\t */\n> > > +\t\tif (child_.status() != MessageReceiver::NoMessage)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tThread::current()->dispatchMessages(Message::None);\n> > > +\n> > > +\t\t/* The child should now have received the message. */\n> > > +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n> > > +\t\t\tsuccess_ = true;\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tMessageReceiver child_;\n> > > +\tbool success_;\n> > > +};\n> > > +\n> > > class SlowMessageReceiver : public Object\n> > > {\n> > > protected:\n> > > @@ -120,6 +159,29 @@ protected:\n> > >\n> > > \t\tdelete slowReceiver;\n> > >\n> > > +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n> > > +\t\t * should be delivered correctly, without crashes or memory\n> > > +\t\t * leaks. Two messages need to be posted to ensure we don't only\n> > > +\t\t * test the simple case of a queue containing a single message.\n> > > +\t\t */\n> > > +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n> > > +\t\trecursiveReceiver->moveToThread(&thread_);\n> > > +\n> > > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n> > > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n> >\n> > I'm not sure why two messages are necessary, also considering that\n> > messages != Message::None are ignored...\n> >\n> > If my understanding is correct, a single postMessage() triggers a\n> > dispatcher->interrupt() which triggers a dispatchMessages() as the receiver\n> > runs on a different thread and can be interrupted. The message is then\n> > dispatched and handled by the execution of RecursiveMessageReceiver::message()\n> > call.\n> >\n> > From there we post a message to the child and go throuh the same path if not\n> > that the child runs on the same thread and we have to dispatch\n> > messages forcefully, causing a recursive call to dispatchMessages() on\n> > the same thread of execution.\n> >\n> > Am I missing something or is the second message not required ?\n>\n> The second message is needed to ensure we don't only test a case where\n> the queue has a single message. The test doesn't fail in that case, due\n> to the fact that the internal iter variable in\n> Thread::dispatchMessages() is incremented before dispatching the current\n> message. If the list contains a single message, iter will thus be the\n> end() of the list after being incremented, which will not cause the\n> outer dispatchMessage() call to fail with use-after-free (the .end()\n> iterator is not invalidated by an erase operation on a std::list).\n>\n\nOh I see, thanks for clarifying this!\n\n> > > +\n> > > +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n> > > +\n> > > +\t\tif (!recursiveReceiver->success()) {\n> > > +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n> > > +\t\t\treturn TestFail;\n\nDoesn't this leak recursiveReceiver ?\nCan it be allocated on the stack or wrapped in a unique_ptr<> ?\n\nWith this fixed\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> > > +\t\t}\n> > > +\n> > > +\t\tdelete recursiveReceiver;\n> > > +\n> > > \t\treturn TestPass;\n> > > \t}\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CA495C0100\n\tfor ;\n\tMon, 5 Jul 2021 12:37:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C06F684E4;\n\tMon, 5 Jul 2021 14:37:24 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27D876050A\n\tfor ;\n\tMon, 5 Jul 2021 14:37:23 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 5B94A24000C;\n\tMon, 5 Jul 2021 12:37:21 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 14:38:11 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>\n\t<20210705110829.ligzn7qhla67v77g@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17985,"web_url":"https://patchwork.libcamera.org/comment/17985/","msgid":"","date":"2021-07-05T12:48:51","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 02:38:11PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:\n> > > On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n> > > > The Thread::dispatchMessages() function needs to support recursive\n> > > > calls, for instance to allow flushing delivery of invoked methods. Add a\n> > > > corresponding test, which currently fails with a double free.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n> > > > 1 file changed, 64 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/test/message.cpp b/test/message.cpp\n> > > > index eeea57feab76..7895cd7c2106 100644\n> > > > --- a/test/message.cpp\n> > > > +++ b/test/message.cpp\n> > > > @@ -26,8 +26,8 @@ public:\n> > > > \t\tMessageReceived,\n> > > > \t};\n> > > >\n> > > > -\tMessageReceiver()\n> > > > -\t\t: status_(NoMessage)\n> > > > +\tMessageReceiver(Object *parent = nullptr)\n> > > > +\t\t: Object(parent), status_(NoMessage)\n> > > > \t{\n> > > > \t}\n> > > >\n> > > > @@ -52,6 +52,45 @@ private:\n> > > > \tStatus status_;\n> > > > };\n> > > >\n> > > > +class RecursiveMessageReceiver : public Object\n> > > > +{\n> > > > +public:\n> > > > +\tRecursiveMessageReceiver()\n> > > > +\t\t: child_(this), success_(false)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > +\tbool success() const { return success_; }\n> > > > +\n> > > > +protected:\n> > > > +\tvoid message([[maybe_unused]] Message *msg)\n> > > > +\t{\n> > > > +\t\tif (msg->type() != Message::None) {\n> > > > +\t\t\tObject::message(msg);\n> > > > +\t\t\treturn;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tchild_.postMessage(std::make_unique(Message::None));\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * If the child has already received the message, something is\n> > > > +\t\t * wrong.\n> > > > +\t\t */\n> > > > +\t\tif (child_.status() != MessageReceiver::NoMessage)\n> > > > +\t\t\treturn;\n> > > > +\n> > > > +\t\tThread::current()->dispatchMessages(Message::None);\n> > > > +\n> > > > +\t\t/* The child should now have received the message. */\n> > > > +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n> > > > +\t\t\tsuccess_ = true;\n> > > > +\t}\n> > > > +\n> > > > +private:\n> > > > +\tMessageReceiver child_;\n> > > > +\tbool success_;\n> > > > +};\n> > > > +\n> > > > class SlowMessageReceiver : public Object\n> > > > {\n> > > > protected:\n> > > > @@ -120,6 +159,29 @@ protected:\n> > > >\n> > > > \t\tdelete slowReceiver;\n> > > >\n> > > > +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n> > > > +\t\t * should be delivered correctly, without crashes or memory\n> > > > +\t\t * leaks. Two messages need to be posted to ensure we don't only\n> > > > +\t\t * test the simple case of a queue containing a single message.\n> > > > +\t\t */\n> > > > +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n> > > > +\t\trecursiveReceiver->moveToThread(&thread_);\n> > > > +\n> > > > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n> > > > +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n> > >\n> > > I'm not sure why two messages are necessary, also considering that\n> > > messages != Message::None are ignored...\n> > >\n> > > If my understanding is correct, a single postMessage() triggers a\n> > > dispatcher->interrupt() which triggers a dispatchMessages() as the receiver\n> > > runs on a different thread and can be interrupted. The message is then\n> > > dispatched and handled by the execution of RecursiveMessageReceiver::message()\n> > > call.\n> > >\n> > > From there we post a message to the child and go throuh the same path if not\n> > > that the child runs on the same thread and we have to dispatch\n> > > messages forcefully, causing a recursive call to dispatchMessages() on\n> > > the same thread of execution.\n> > >\n> > > Am I missing something or is the second message not required ?\n> >\n> > The second message is needed to ensure we don't only test a case where\n> > the queue has a single message. The test doesn't fail in that case, due\n> > to the fact that the internal iter variable in\n> > Thread::dispatchMessages() is incremented before dispatching the current\n> > message. If the list contains a single message, iter will thus be the\n> > end() of the list after being incremented, which will not cause the\n> > outer dispatchMessage() call to fail with use-after-free (the .end()\n> > iterator is not invalidated by an erase operation on a std::list).\n> \n> Oh I see, thanks for clarifying this!\n> \n> > > > +\n> > > > +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n> > > > +\n> > > > +\t\tif (!recursiveReceiver->success()) {\n> > > > +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n> > > > +\t\t\treturn TestFail;\n> \n> Doesn't this leak recursiveReceiver ?\n> Can it be allocated on the stack or wrapped in a unique_ptr<> ?\n\nYou're right. I initially thought it was just in an error path in a\ntest, but that matters too. I'll use a unique_ptr.\n\n> With this fixed\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > > > +\t\t}\n> > > > +\n> > > > +\t\tdelete recursiveReceiver;\n> > > > +\n> > > > \t\treturn TestPass;\n> > > > \t}\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBA85BD794\n\tfor ;\n\tMon, 5 Jul 2021 12:49:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49087684F9;\n\tMon, 5 Jul 2021 14:49:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 482D46050A\n\tfor ;\n\tMon, 5 Jul 2021 14:49:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B8CD8880;\n\tMon, 5 Jul 2021 14:49:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oJTCeUWv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625489373;\n\tbh=9bS5O99HrEyigtLwu72bZUU+JrRV6YlKidvbOlQHd7w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oJTCeUWvFHRR784CZvdmlDMQf+O9l4MmqbdInUSRIEpEZ65nzL/1B7HWOE+dEwDCE\n\tc1jCgw772CzN6JjuUga9+YRbOgETCqXEfgHGEUZbiToBf//pPwL6M5j5m5WXcwkL9i\n\trZ5N0/mIxq/YBdAB4WR2mSfhaKoaR9He3y1NopIY=","Date":"Mon, 5 Jul 2021 15:48:51 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>\n\t<20210705110829.ligzn7qhla67v77g@uno.localdomain>\n\t\n\t<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18042,"web_url":"https://patchwork.libcamera.org/comment/18042/","msgid":"<7239e6c6-dff8-81cc-214b-a03e42abba16@ideasonboard.com>","date":"2021-07-09T09:57:23","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nReplying here to follow existing discussions...\n\n\nOn 05/07/2021 13:48, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Mon, Jul 05, 2021 at 02:38:11PM +0200, Jacopo Mondi wrote:\n>> On Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:\n>>> On Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:\n>>>> On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n>>>>> The Thread::dispatchMessages() function needs to support recursive\n>>>>> calls, for instance to allow flushing delivery of invoked methods. Add a\n>>>>> corresponding test, which currently fails with a double free.\n>>>>>\n>>>>> Signed-off-by: Laurent Pinchart \n>>>>> ---\n>>>>> test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n>>>>> 1 file changed, 64 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/test/message.cpp b/test/message.cpp\n>>>>> index eeea57feab76..7895cd7c2106 100644\n>>>>> --- a/test/message.cpp\n>>>>> +++ b/test/message.cpp\n>>>>> @@ -26,8 +26,8 @@ public:\n>>>>> \t\tMessageReceived,\n>>>>> \t};\n>>>>>\n>>>>> -\tMessageReceiver()\n>>>>> -\t\t: status_(NoMessage)\n>>>>> +\tMessageReceiver(Object *parent = nullptr)\n>>>>> +\t\t: Object(parent), status_(NoMessage)\n>>>>> \t{\n>>>>> \t}\n>>>>>\n>>>>> @@ -52,6 +52,45 @@ private:\n>>>>> \tStatus status_;\n>>>>> };\n>>>>>\n>>>>> +class RecursiveMessageReceiver : public Object\n>>>>> +{\n>>>>> +public:\n>>>>> +\tRecursiveMessageReceiver()\n>>>>> +\t\t: child_(this), success_(false)\n>>>>> +\t{\n>>>>> +\t}\n>>>>> +\n>>>>> +\tbool success() const { return success_; }\n>>>>> +\n>>>>> +protected:\n>>>>> +\tvoid message([[maybe_unused]] Message *msg)\n>>>>> +\t{\n>>>>> +\t\tif (msg->type() != Message::None) {\n>>>>> +\t\t\tObject::message(msg);\n>>>>> +\t\t\treturn;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tchild_.postMessage(std::make_unique(Message::None));\n>>>>> +\n>>>>> +\t\t/*\n>>>>> +\t\t * If the child has already received the message, something is\n>>>>> +\t\t * wrong.\n>>>>> +\t\t */\n>>>>> +\t\tif (child_.status() != MessageReceiver::NoMessage)\n>>>>> +\t\t\treturn;\n>>>>> +\n>>>>> +\t\tThread::current()->dispatchMessages(Message::None);\n>>>>> +\n>>>>> +\t\t/* The child should now have received the message. */\n>>>>> +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n>>>>> +\t\t\tsuccess_ = true;\n>>>>> +\t}\n>>>>> +\n>>>>> +private:\n>>>>> +\tMessageReceiver child_;\n>>>>> +\tbool success_;\n>>>>> +};\n>>>>> +\n>>>>> class SlowMessageReceiver : public Object\n>>>>> {\n>>>>> protected:\n>>>>> @@ -120,6 +159,29 @@ protected:\n>>>>>\n>>>>> \t\tdelete slowReceiver;\n>>>>>\n>>>>> +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n>>>>> +\n>>>>> +\t\t/*\n>>>>> +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n>>>>> +\t\t * should be delivered correctly, without crashes or memory\n>>>>> +\t\t * leaks. Two messages need to be posted to ensure we don't only\n>>>>> +\t\t * test the simple case of a queue containing a single message.\n>>>>> +\t\t */\n>>>>> +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n>>>>> +\t\trecursiveReceiver->moveToThread(&thread_);\n>>>>> +\n>>>>> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n>>>>> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n>>>>\n>>>> I'm not sure why two messages are necessary, also considering that\n>>>> messages != Message::None are ignored...\n>>>>\n>>>> If my understanding is correct, a single postMessage() triggers a\n>>>> dispatcher->interrupt() which triggers a dispatchMessages() as the receiver\n>>>> runs on a different thread and can be interrupted. The message is then\n>>>> dispatched and handled by the execution of RecursiveMessageReceiver::message()\n>>>> call.\n>>>>\n>>>> From there we post a message to the child and go throuh the same path if not\n>>>> that the child runs on the same thread and we have to dispatch\n>>>> messages forcefully, causing a recursive call to dispatchMessages() on\n>>>> the same thread of execution.\n>>>>\n>>>> Am I missing something or is the second message not required ?\n>>>\n>>> The second message is needed to ensure we don't only test a case where\n>>> the queue has a single message. The test doesn't fail in that case, due\n>>> to the fact that the internal iter variable in\n>>> Thread::dispatchMessages() is incremented before dispatching the current\n>>> message. If the list contains a single message, iter will thus be the\n>>> end() of the list after being incremented, which will not cause the\n>>> outer dispatchMessage() call to fail with use-after-free (the .end()\n>>> iterator is not invalidated by an erase operation on a std::list).\n>>\n>> Oh I see, thanks for clarifying this!\n\nAre there any further edge cases here? I.e. should we post three\nmessages to ensure that they all clean up iteratively?\n\nI am only suggesting this in case, conceptually there are two overlaps -\nwhere three messages would ensure an overlap for the both the beginning\nand end of message two for instance..\n\n\n\n \n \n\n\n>>>>> +\n>>>>> +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n>>>>> +\n>>>>> +\t\tif (!recursiveReceiver->success()) {\n>>>>> +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n>>>>> +\t\t\treturn TestFail;\n>>\n>> Doesn't this leak recursiveReceiver ?\n>> Can it be allocated on the stack or wrapped in a unique_ptr<> ?\n> \n> You're right. I initially thought it was just in an error path in a\n> test, but that matters too. I'll use a unique_ptr.\n> \n>> With this fixed\n>>\n>> Reviewed-by: Jacopo Mondi \n\nYup, I'll go along with that.\n\nReviewed-by: Kieran Bingham \n\n\n\n>>\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tdelete recursiveReceiver;\n>>>>> +\n>>>>> \t\treturn TestPass;\n>>>>> \t}\n>>>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19463BD794\n\tfor ;\n\tFri, 9 Jul 2021 09:57:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D14866851F;\n\tFri, 9 Jul 2021 11:57:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 402DD684F3\n\tfor ;\n\tFri, 9 Jul 2021 11:57:26 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC9FDE7;\n\tFri, 9 Jul 2021 11:57:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SX1nNvO1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625824645;\n\tbh=TIDEvJ8ZpXtc+SlLl8ff/zwGBI4DRf1Z5MqkG0a2ajU=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=SX1nNvO1VDluXpUcAoNRqi9su+KDvYSpQZyb8bfrwmGV9x7IPUDe5YHLrkHN79gQe\n\tVqSEffOnQMe0EPhyYqXJN+bwc2te1iQ90pjqyeAir/AQc4ouCoU/oqdC1CQ0RGQsPp\n\tcty6dE3cvqRGK/Cd4OUafLZ9T2bawARP6ZO8y1aU=","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>\n\t<20210705110829.ligzn7qhla67v77g@uno.localdomain>\n\t\n\t<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>\n\t","From":"Kieran Bingham ","Message-ID":"<7239e6c6-dff8-81cc-214b-a03e42abba16@ideasonboard.com>","Date":"Fri, 9 Jul 2021 10:57:23 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18072,"web_url":"https://patchwork.libcamera.org/comment/18072/","msgid":"","date":"2021-07-11T14:21:42","subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Jul 09, 2021 at 10:57:23AM +0100, Kieran Bingham wrote:\n> On 05/07/2021 13:48, Laurent Pinchart wrote:\n> > On Mon, Jul 05, 2021 at 02:38:11PM +0200, Jacopo Mondi wrote:\n> >> On Mon, Jul 05, 2021 at 03:16:32PM +0300, Laurent Pinchart wrote:\n> >>> On Mon, Jul 05, 2021 at 01:08:29PM +0200, Jacopo Mondi wrote:\n> >>>> On Fri, Jul 02, 2021 at 02:07:40AM +0300, Laurent Pinchart wrote:\n> >>>>> The Thread::dispatchMessages() function needs to support recursive\n> >>>>> calls, for instance to allow flushing delivery of invoked methods. Add a\n> >>>>> corresponding test, which currently fails with a double free.\n> >>>>>\n> >>>>> Signed-off-by: Laurent Pinchart \n> >>>>> ---\n> >>>>> test/message.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++--\n> >>>>> 1 file changed, 64 insertions(+), 2 deletions(-)\n> >>>>>\n> >>>>> diff --git a/test/message.cpp b/test/message.cpp\n> >>>>> index eeea57feab76..7895cd7c2106 100644\n> >>>>> --- a/test/message.cpp\n> >>>>> +++ b/test/message.cpp\n> >>>>> @@ -26,8 +26,8 @@ public:\n> >>>>> \t\tMessageReceived,\n> >>>>> \t};\n> >>>>>\n> >>>>> -\tMessageReceiver()\n> >>>>> -\t\t: status_(NoMessage)\n> >>>>> +\tMessageReceiver(Object *parent = nullptr)\n> >>>>> +\t\t: Object(parent), status_(NoMessage)\n> >>>>> \t{\n> >>>>> \t}\n> >>>>>\n> >>>>> @@ -52,6 +52,45 @@ private:\n> >>>>> \tStatus status_;\n> >>>>> };\n> >>>>>\n> >>>>> +class RecursiveMessageReceiver : public Object\n> >>>>> +{\n> >>>>> +public:\n> >>>>> +\tRecursiveMessageReceiver()\n> >>>>> +\t\t: child_(this), success_(false)\n> >>>>> +\t{\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +\tbool success() const { return success_; }\n> >>>>> +\n> >>>>> +protected:\n> >>>>> +\tvoid message([[maybe_unused]] Message *msg)\n> >>>>> +\t{\n> >>>>> +\t\tif (msg->type() != Message::None) {\n> >>>>> +\t\t\tObject::message(msg);\n> >>>>> +\t\t\treturn;\n> >>>>> +\t\t}\n> >>>>> +\n> >>>>> +\t\tchild_.postMessage(std::make_unique(Message::None));\n> >>>>> +\n> >>>>> +\t\t/*\n> >>>>> +\t\t * If the child has already received the message, something is\n> >>>>> +\t\t * wrong.\n> >>>>> +\t\t */\n> >>>>> +\t\tif (child_.status() != MessageReceiver::NoMessage)\n> >>>>> +\t\t\treturn;\n> >>>>> +\n> >>>>> +\t\tThread::current()->dispatchMessages(Message::None);\n> >>>>> +\n> >>>>> +\t\t/* The child should now have received the message. */\n> >>>>> +\t\tif (child_.status() == MessageReceiver::MessageReceived)\n> >>>>> +\t\t\tsuccess_ = true;\n> >>>>> +\t}\n> >>>>> +\n> >>>>> +private:\n> >>>>> +\tMessageReceiver child_;\n> >>>>> +\tbool success_;\n> >>>>> +};\n> >>>>> +\n> >>>>> class SlowMessageReceiver : public Object\n> >>>>> {\n> >>>>> protected:\n> >>>>> @@ -120,6 +159,29 @@ protected:\n> >>>>>\n> >>>>> \t\tdelete slowReceiver;\n> >>>>>\n> >>>>> +\t\tthis_thread::sleep_for(chrono::milliseconds(100));\n> >>>>> +\n> >>>>> +\t\t/*\n> >>>>> +\t\t * Test recursive calls to Thread::dispatchMessages(). Messages\n> >>>>> +\t\t * should be delivered correctly, without crashes or memory\n> >>>>> +\t\t * leaks. Two messages need to be posted to ensure we don't only\n> >>>>> +\t\t * test the simple case of a queue containing a single message.\n> >>>>> +\t\t */\n> >>>>> +\t\tRecursiveMessageReceiver *recursiveReceiver = new RecursiveMessageReceiver();\n> >>>>> +\t\trecursiveReceiver->moveToThread(&thread_);\n> >>>>> +\n> >>>>> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::None));\n> >>>>> +\t\trecursiveReceiver->postMessage(std::make_unique(Message::UserMessage));\n> >>>>\n> >>>> I'm not sure why two messages are necessary, also considering that\n> >>>> messages != Message::None are ignored...\n> >>>>\n> >>>> If my understanding is correct, a single postMessage() triggers a\n> >>>> dispatcher->interrupt() which triggers a dispatchMessages() as the receiver\n> >>>> runs on a different thread and can be interrupted. The message is then\n> >>>> dispatched and handled by the execution of RecursiveMessageReceiver::message()\n> >>>> call.\n> >>>>\n> >>>> From there we post a message to the child and go throuh the same path if not\n> >>>> that the child runs on the same thread and we have to dispatch\n> >>>> messages forcefully, causing a recursive call to dispatchMessages() on\n> >>>> the same thread of execution.\n> >>>>\n> >>>> Am I missing something or is the second message not required ?\n> >>>\n> >>> The second message is needed to ensure we don't only test a case where\n> >>> the queue has a single message. The test doesn't fail in that case, due\n> >>> to the fact that the internal iter variable in\n> >>> Thread::dispatchMessages() is incremented before dispatching the current\n> >>> message. If the list contains a single message, iter will thus be the\n> >>> end() of the list after being incremented, which will not cause the\n> >>> outer dispatchMessage() call to fail with use-after-free (the .end()\n> >>> iterator is not invalidated by an erase operation on a std::list).\n> >>\n> >> Oh I see, thanks for clarifying this!\n> \n> Are there any further edge cases here? I.e. should we post three\n> messages to ensure that they all clean up iteratively?\n> \n> I am only suggesting this in case, conceptually there are two overlaps -\n> where three messages would ensure an overlap for the both the beginning\n> and end of message two for instance..\n> \n> \n> \n> \n> \n> \n\nIt's a good question. While tests are meant to verify that the\nimplementation is comformant with the API, they embed some knowledge of\nexpected implementation errors. In this case, I'm not sure what issue\nyou think three messages would catch. I'm also not sure what you mean by\noverlap.\n\n> >>>>> +\n> >>>>> +\t\tthis_thread::sleep_for(chrono::milliseconds(10));\n> >>>>> +\n> >>>>> +\t\tif (!recursiveReceiver->success()) {\n> >>>>> +\t\t\tcout << \"Recursive message delivery failed\" << endl;\n> >>>>> +\t\t\treturn TestFail;\n> >>\n> >> Doesn't this leak recursiveReceiver ?\n> >> Can it be allocated on the stack or wrapped in a unique_ptr<> ?\n> > \n> > You're right. I initially thought it was just in an error path in a\n> > test, but that matters too. I'll use a unique_ptr.\n> > \n> >> With this fixed\n> >>\n> >> Reviewed-by: Jacopo Mondi \n> \n> Yup, I'll go along with that.\n> \n> Reviewed-by: Kieran Bingham \n> \n> >>>>> +\t\t}\n> >>>>> +\n> >>>>> +\t\tdelete recursiveReceiver;\n> >>>>> +\n> >>>>> \t\treturn TestPass;\n> >>>>> \t}\n> >>>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9E82FC3224\n\tfor ;\n\tSun, 11 Jul 2021 14:22:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D675E6851D;\n\tSun, 11 Jul 2021 16:22:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8DB9B68519\n\tfor ;\n\tSun, 11 Jul 2021 16:22:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E8418CC;\n\tSun, 11 Jul 2021 16:22:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sbcnf/zF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626013349;\n\tbh=87usnvPFJx2dNZzRWjHXPZ+sAn0iwDBvpxtVIoFMULs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sbcnf/zFd/J3BxfEFwvU0AtlJxisuYakMoRXOJMPkCSAYa0YKhPey8SrykslwSvny\n\tQFHQO2e0oNvkluzQXeSc8C2JcZmLbTUfJ9UW7YieC8gQDoCYNMDLu6Ua5M87MbYDnq\n\t5w59nVe2Z8GupYsajVw0YoYWh4rDqXwYyzxYQw4E=","Date":"Sun, 11 Jul 2021 17:21:42 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-3-laurent.pinchart@ideasonboard.com>\n\t<20210705110829.ligzn7qhla67v77g@uno.localdomain>\n\t\n\t<20210705123811.h6m6kmral7qnjdvq@uno.localdomain>\n\t\n\t<7239e6c6-dff8-81cc-214b-a03e42abba16@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7239e6c6-dff8-81cc-214b-a03e42abba16@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] test: message: Test recursive\n\tThread::dispatchMessages() calls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]