[{"id":17975,"web_url":"https://patchwork.libcamera.org/comment/17975/","msgid":"<20210705104811.eguavrps6mflgcnr@uno.localdomain>","date":"2021-07-05T10:48:11","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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:41AM +0300, Laurent Pinchart wrote:\n> There are use cases for calling the dispatchMessages() function\n> recursively, from within a message handler. This can be used, for\n> instance, to force delivery of messages posted to a thread concurrently\n> to stopping the thread. This currently causes access, in the outer\n> dispatchMessages() call, to iterators that have been invalidated by\n> erasing list elements in the recursive call, leading to undefined\n> behaviour (most likely double-free or other crashes).\n>\n> Fix it by only erasing messages from the list at the end of the outer\n> call, identified using a recursion counter. We need to keep track of the\n> first non-null entry in the posted messages list to restrict the\n> deletion to the\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n> 1 file changed, 33 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 7f79115222e8..9eb488d46db8 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -126,6 +126,11 @@ public:\n> \t * \\brief Protects the \\ref list_\n> \t */\n> \tMutex mutex_;\n> +\t/**\n> +\t * \\brief The recursion level for recursive Thread::dispatchMessages()\n> +\t * calls\n> +\t */\n> +\tunsigned int recursion_ = 0;\n> };\n>\n> /**\n> @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n> * Messages shall only be dispatched from the current thread, typically within\n> * the thread from the run() function. Calling this function outside of the\n> * thread results in undefined behaviour.\n> + *\n> + * This function is not thread-safe, but it may be called recursively in the\n> + * same thread from an object's message handler. It guarantees in all cases\n> + * delivery of messages in the order they have been posted.\n> */\n> void Thread::dispatchMessages(Message::Type type)\n> {\n> \tASSERT(data_ == ThreadData::current());\n>\n> +\t++data_->messages_.recursion_;\n> +\n> \tMutexLocker locker(data_->messages_.mutex_);\n>\n> \tstd::list> &messages = data_->messages_.list_;\n>\n> -\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> -\t\tstd::unique_ptr &msg = *iter;\n> -\n> -\t\tif (!msg) {\n> -\t\t\titer = data_->messages_.list_.erase(iter);\n> +\tfor (std::unique_ptr &msg : messages) {\n> +\t\tif (!msg)\n> \t\t\tcontinue;\n> -\t\t}\n>\n> -\t\tif (type != Message::Type::None && msg->type() != type) {\n> -\t\t\t++iter;\n> +\t\tif (type != Message::Type::None && msg->type() != type)\n> \t\t\tcontinue;\n> -\t\t}\n>\n> +\t\t/*\n> +\t\t * Move the message, setting the entry in the list to null. It\n> +\t\t * will cause recursive calls to ignore the entry, and the erase\n> +\t\t * loop at the end of the function to delete it from the list.\n> +\t\t */\n> \t\tstd::unique_ptr message = std::move(msg);\n> -\t\titer = data_->messages_.list_.erase(iter);\n>\n> \t\tObject *receiver = message->receiver_;\n> \t\tASSERT(data_ == receiver->thread()->data_);\n> @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n> \t\tmessage.reset();\n> \t\tlocker.lock();\n> \t}\n> +\n> +\t/*\n> +\t * If the recursion level is 0, erase all null messages in the list. We\n> +\t * can't do so during recursion, as it would invalidate the iterator of\n> +\t * the outer calls.\n> +\t */\n\nis it paranoid to think accesses to recursions_ should be serialized ?\n\n> +\tif (!--data_->messages_.recursion_) {\n> +\t\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> +\t\t\tif (!*iter)\n> +\t\t\t\titer = messages.erase(iter);\n> +\t\t\telse\n> +\t\t\t\t++iter;\n> +\t\t}\n> +\t}\n\nCan null messages end up in the list ? can't you just clear() the\nwhole list ?\n\nThanks\n j\n\n> }\n>\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 3046FC0100\n\tfor ;\n\tMon, 5 Jul 2021 10:47:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7F07684F9;\n\tMon, 5 Jul 2021 12:47:23 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F92D6050A\n\tfor ;\n\tMon, 5 Jul 2021 12:47:22 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 24521FF808;\n\tMon, 5 Jul 2021 10:47:21 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 12:48:11 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705104811.eguavrps6mflgcnr@uno.localdomain>","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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":17979,"web_url":"https://patchwork.libcamera.org/comment/17979/","msgid":"","date":"2021-07-05T12:12:45","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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 12:48:11PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 02, 2021 at 02:07:41AM +0300, Laurent Pinchart wrote:\n> > There are use cases for calling the dispatchMessages() function\n> > recursively, from within a message handler. This can be used, for\n> > instance, to force delivery of messages posted to a thread concurrently\n> > to stopping the thread. This currently causes access, in the outer\n> > dispatchMessages() call, to iterators that have been invalidated by\n> > erasing list elements in the recursive call, leading to undefined\n> > behaviour (most likely double-free or other crashes).\n> >\n> > Fix it by only erasing messages from the list at the end of the outer\n> > call, identified using a recursion counter. We need to keep track of the\n> > first non-null entry in the posted messages list to restrict the\n> > deletion to the\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n> > 1 file changed, 33 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index 7f79115222e8..9eb488d46db8 100644\n> > --- a/src/libcamera/base/thread.cpp\n> > +++ b/src/libcamera/base/thread.cpp\n> > @@ -126,6 +126,11 @@ public:\n> > \t * \\brief Protects the \\ref list_\n> > \t */\n> > \tMutex mutex_;\n> > +\t/**\n> > +\t * \\brief The recursion level for recursive Thread::dispatchMessages()\n> > +\t * calls\n> > +\t */\n> > +\tunsigned int recursion_ = 0;\n> > };\n> >\n> > /**\n> > @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n> > * Messages shall only be dispatched from the current thread, typically within\n> > * the thread from the run() function. Calling this function outside of the\n> > * thread results in undefined behaviour.\n> > + *\n> > + * This function is not thread-safe, but it may be called recursively in the\n> > + * same thread from an object's message handler. It guarantees in all cases\n> > + * delivery of messages in the order they have been posted.\n> > */\n> > void Thread::dispatchMessages(Message::Type type)\n> > {\n> > \tASSERT(data_ == ThreadData::current());\n> >\n> > +\t++data_->messages_.recursion_;\n> > +\n> > \tMutexLocker locker(data_->messages_.mutex_);\n> >\n> > \tstd::list> &messages = data_->messages_.list_;\n> >\n> > -\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> > -\t\tstd::unique_ptr &msg = *iter;\n> > -\n> > -\t\tif (!msg) {\n> > -\t\t\titer = data_->messages_.list_.erase(iter);\n> > +\tfor (std::unique_ptr &msg : messages) {\n> > +\t\tif (!msg)\n> > \t\t\tcontinue;\n> > -\t\t}\n> >\n> > -\t\tif (type != Message::Type::None && msg->type() != type) {\n> > -\t\t\t++iter;\n> > +\t\tif (type != Message::Type::None && msg->type() != type)\n> > \t\t\tcontinue;\n> > -\t\t}\n> >\n> > +\t\t/*\n> > +\t\t * Move the message, setting the entry in the list to null. It\n> > +\t\t * will cause recursive calls to ignore the entry, and the erase\n> > +\t\t * loop at the end of the function to delete it from the list.\n> > +\t\t */\n> > \t\tstd::unique_ptr message = std::move(msg);\n> > -\t\titer = data_->messages_.list_.erase(iter);\n> >\n> > \t\tObject *receiver = message->receiver_;\n> > \t\tASSERT(data_ == receiver->thread()->data_);\n> > @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n> > \t\tmessage.reset();\n> > \t\tlocker.lock();\n> > \t}\n> > +\n> > +\t/*\n> > +\t * If the recursion level is 0, erase all null messages in the list. We\n> > +\t * can't do so during recursion, as it would invalidate the iterator of\n> > +\t * the outer calls.\n> > +\t */\n> \n> is it paranoid to think accesses to recursions_ should be serialized ?\n\nThe function is documented as not being thread-safe. This is because it\nmust be called from the thread created by the Thread class (see the\nassert at the beginning that enforces that). While it can be called\nrecursively, there will never be two concurrent calls, so no race\ncondition when accessing the recursion_ variable. This is unlike the\nmessages list that needs to be protected by a lock because other threads\ncan post messages to the list concurrently to\nThread::dispatchMessages().\n\n> > +\tif (!--data_->messages_.recursion_) {\n> > +\t\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> > +\t\t\tif (!*iter)\n> > +\t\t\t\titer = messages.erase(iter);\n> > +\t\t\telse\n> > +\t\t\t\t++iter;\n> > +\t\t}\n> > +\t}\n> \n> Can null messages end up in the list ? can't you just clear() the\n> whole list ?\n\nThe function takes a message type parameter to restrict it to\ndispatching messages of a given type. The list may thus not be empty, it\nmay still contain messages of other types.\n\n> > }\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 CF1A6C0100\n\tfor ;\n\tMon, 5 Jul 2021 12:13:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3BAD2684F9;\n\tMon, 5 Jul 2021 14:13:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1D756050A\n\tfor ;\n\tMon, 5 Jul 2021 14:13:27 +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 66360880;\n\tMon, 5 Jul 2021 14:13:27 +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=\"NxLZrIox\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625487207;\n\tbh=3fgku/Ln8LN7jVkLREvAczMVxhJbAO6UGSqkiUrykEo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NxLZrIoxqcp0WJ9AwBOK+oEOiQbTSkKsynx3XHuNVLkgc11LhAxo9GWt0363h0dfQ\n\tVzDBt9de1vy6Y0JTIQRow/RqJOhHpYoMpThsNMIVPWSuqB5Ba+IILtkPjzRq8DsQkE\n\t02/qEgeYH6BHB5ypOxzQspH5fFpSR2FFqbq1axNc=","Date":"Mon, 5 Jul 2021 15:12:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>\n\t<20210705104811.eguavrps6mflgcnr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210705104811.eguavrps6mflgcnr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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":17981,"web_url":"https://patchwork.libcamera.org/comment/17981/","msgid":"<20210705123303.ncen4quzugvyzmxa@uno.localdomain>","date":"2021-07-05T12:33:03","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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:12:45PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 05, 2021 at 12:48:11PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 02, 2021 at 02:07:41AM +0300, Laurent Pinchart wrote:\n> > > There are use cases for calling the dispatchMessages() function\n> > > recursively, from within a message handler. This can be used, for\n> > > instance, to force delivery of messages posted to a thread concurrently\n> > > to stopping the thread. This currently causes access, in the outer\n> > > dispatchMessages() call, to iterators that have been invalidated by\n> > > erasing list elements in the recursive call, leading to undefined\n> > > behaviour (most likely double-free or other crashes).\n> > >\n> > > Fix it by only erasing messages from the list at the end of the outer\n> > > call, identified using a recursion counter. We need to keep track of the\n> > > first non-null entry in the posted messages list to restrict the\n> > > deletion to the\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n> > > 1 file changed, 33 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > index 7f79115222e8..9eb488d46db8 100644\n> > > --- a/src/libcamera/base/thread.cpp\n> > > +++ b/src/libcamera/base/thread.cpp\n> > > @@ -126,6 +126,11 @@ public:\n> > > \t * \\brief Protects the \\ref list_\n> > > \t */\n> > > \tMutex mutex_;\n> > > +\t/**\n> > > +\t * \\brief The recursion level for recursive Thread::dispatchMessages()\n> > > +\t * calls\n> > > +\t */\n> > > +\tunsigned int recursion_ = 0;\n> > > };\n> > >\n> > > /**\n> > > @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n> > > * Messages shall only be dispatched from the current thread, typically within\n> > > * the thread from the run() function. Calling this function outside of the\n> > > * thread results in undefined behaviour.\n> > > + *\n> > > + * This function is not thread-safe, but it may be called recursively in the\n> > > + * same thread from an object's message handler. It guarantees in all cases\n> > > + * delivery of messages in the order they have been posted.\n> > > */\n> > > void Thread::dispatchMessages(Message::Type type)\n> > > {\n> > > \tASSERT(data_ == ThreadData::current());\n> > >\n> > > +\t++data_->messages_.recursion_;\n> > > +\n> > > \tMutexLocker locker(data_->messages_.mutex_);\n> > >\n> > > \tstd::list> &messages = data_->messages_.list_;\n> > >\n> > > -\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> > > -\t\tstd::unique_ptr &msg = *iter;\n> > > -\n> > > -\t\tif (!msg) {\n> > > -\t\t\titer = data_->messages_.list_.erase(iter);\n> > > +\tfor (std::unique_ptr &msg : messages) {\n> > > +\t\tif (!msg)\n> > > \t\t\tcontinue;\n> > > -\t\t}\n> > >\n> > > -\t\tif (type != Message::Type::None && msg->type() != type) {\n> > > -\t\t\t++iter;\n> > > +\t\tif (type != Message::Type::None && msg->type() != type)\n> > > \t\t\tcontinue;\n> > > -\t\t}\n> > >\n> > > +\t\t/*\n> > > +\t\t * Move the message, setting the entry in the list to null. It\n> > > +\t\t * will cause recursive calls to ignore the entry, and the erase\n> > > +\t\t * loop at the end of the function to delete it from the list.\n> > > +\t\t */\n> > > \t\tstd::unique_ptr message = std::move(msg);\n> > > -\t\titer = data_->messages_.list_.erase(iter);\n> > >\n> > > \t\tObject *receiver = message->receiver_;\n> > > \t\tASSERT(data_ == receiver->thread()->data_);\n> > > @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n> > > \t\tmessage.reset();\n> > > \t\tlocker.lock();\n> > > \t}\n> > > +\n> > > +\t/*\n> > > +\t * If the recursion level is 0, erase all null messages in the list. We\n> > > +\t * can't do so during recursion, as it would invalidate the iterator of\n> > > +\t * the outer calls.\n> > > +\t */\n> >\n> > is it paranoid to think accesses to recursions_ should be serialized ?\n>\n> The function is documented as not being thread-safe. This is because it\n> must be called from the thread created by the Thread class (see the\n> assert at the beginning that enforces that). While it can be called\n> recursively, there will never be two concurrent calls, so no race\n> condition when accessing the recursion_ variable. This is unlike the\n> messages list that needs to be protected by a lock because other threads\n> can post messages to the list concurrently to\n> Thread::dispatchMessages().\n>\n> > > +\tif (!--data_->messages_.recursion_) {\n> > > +\t\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> > > +\t\t\tif (!*iter)\n> > > +\t\t\t\titer = messages.erase(iter);\n> > > +\t\t\telse\n> > > +\t\t\t\t++iter;\n> > > +\t\t}\n> > > +\t}\n> >\n> > Can null messages end up in the list ? can't you just clear() the\n> > whole list ?\n>\n> The function takes a message type parameter to restrict it to\n> dispatching messages of a given type. The list may thus not be empty, it\n> may still contain messages of other types.\n\nSo it is the other way around actually, if the message has been\ndispatched, we erase it. Sorry, I read the condition wrong, thanks for\nexplaining.\n\nA little note, you could:\n\n if (--data_->messages_.recursion_)\n return;\n\nAnd save one indentation level.\n\nApart from this:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> > > }\n> > >\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 480E1BD794\n\tfor ;\n\tMon, 5 Jul 2021 12:32:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EAA468503;\n\tMon, 5 Jul 2021 14:32:17 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F5546050A\n\tfor ;\n\tMon, 5 Jul 2021 14:32:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 80858240012;\n\tMon, 5 Jul 2021 12:32:14 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 14:33:03 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705123303.ncen4quzugvyzmxa@uno.localdomain>","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>\n\t<20210705104811.eguavrps6mflgcnr@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 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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":17997,"web_url":"https://patchwork.libcamera.org/comment/17997/","msgid":"","date":"2021-07-06T10:07:54","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThank you very much for this patch.\n\nOn Fri, 2 Jul 2021 at 00:08, Laurent Pinchart\n wrote:\n>\n> There are use cases for calling the dispatchMessages() function\n> recursively, from within a message handler. This can be used, for\n> instance, to force delivery of messages posted to a thread concurrently\n> to stopping the thread. This currently causes access, in the outer\n> dispatchMessages() call, to iterators that have been invalidated by\n> erasing list elements in the recursive call, leading to undefined\n> behaviour (most likely double-free or other crashes).\n>\n> Fix it by only erasing messages from the list at the end of the outer\n> call, identified using a recursion counter. We need to keep track of the\n> first non-null entry in the posted messages list to restrict the\n> deletion to the\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n> Signed-off-by: Laurent Pinchart \n\nNotwithstanding the subsequent discussion, I've been running with\nthese changes for a while now and haven't seen the problem return, so:\n\nTested-by: David Plowman \n\nThank you!\nDavid\n\n> ---\n> src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n> 1 file changed, 33 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 7f79115222e8..9eb488d46db8 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -126,6 +126,11 @@ public:\n> * \\brief Protects the \\ref list_\n> */\n> Mutex mutex_;\n> + /**\n> + * \\brief The recursion level for recursive Thread::dispatchMessages()\n> + * calls\n> + */\n> + unsigned int recursion_ = 0;\n> };\n>\n> /**\n> @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n> * Messages shall only be dispatched from the current thread, typically within\n> * the thread from the run() function. Calling this function outside of the\n> * thread results in undefined behaviour.\n> + *\n> + * This function is not thread-safe, but it may be called recursively in the\n> + * same thread from an object's message handler. It guarantees in all cases\n> + * delivery of messages in the order they have been posted.\n> */\n> void Thread::dispatchMessages(Message::Type type)\n> {\n> ASSERT(data_ == ThreadData::current());\n>\n> + ++data_->messages_.recursion_;\n> +\n> MutexLocker locker(data_->messages_.mutex_);\n>\n> std::list> &messages = data_->messages_.list_;\n>\n> - for (auto iter = messages.begin(); iter != messages.end(); ) {\n> - std::unique_ptr &msg = *iter;\n> -\n> - if (!msg) {\n> - iter = data_->messages_.list_.erase(iter);\n> + for (std::unique_ptr &msg : messages) {\n> + if (!msg)\n> continue;\n> - }\n>\n> - if (type != Message::Type::None && msg->type() != type) {\n> - ++iter;\n> + if (type != Message::Type::None && msg->type() != type)\n> continue;\n> - }\n>\n> + /*\n> + * Move the message, setting the entry in the list to null. It\n> + * will cause recursive calls to ignore the entry, and the erase\n> + * loop at the end of the function to delete it from the list.\n> + */\n> std::unique_ptr message = std::move(msg);\n> - iter = data_->messages_.list_.erase(iter);\n>\n> Object *receiver = message->receiver_;\n> ASSERT(data_ == receiver->thread()->data_);\n> @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n> message.reset();\n> locker.lock();\n> }\n> +\n> + /*\n> + * If the recursion level is 0, erase all null messages in the list. We\n> + * can't do so during recursion, as it would invalidate the iterator of\n> + * the outer calls.\n> + */\n> + if (!--data_->messages_.recursion_) {\n> + for (auto iter = messages.begin(); iter != messages.end(); ) {\n> + if (!*iter)\n> + iter = messages.erase(iter);\n> + else\n> + ++iter;\n> + }\n> + }\n> }\n>\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 DC670C3224\n\tfor ;\n\tTue, 6 Jul 2021 10:08:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B38168502;\n\tTue, 6 Jul 2021 12:08:07 +0200 (CEST)","from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com\n\t[IPv6:2a00:1450:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D15A060288\n\tfor ;\n\tTue, 6 Jul 2021 12:08:05 +0200 (CEST)","by mail-wm1-x32b.google.com with SMTP id n33so5682435wms.1\n\tfor ;\n\tTue, 06 Jul 2021 03:08:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"g1Fhi/CD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=LZt3AiEa0gsDsKn5d8+E3QLwkkRx473N0P3vai9/zlk=;\n\tb=g1Fhi/CDe8AQLnSTQPOM0WbAEvz56xXmz3qvAY/dkjzrQxn0L8hFZItHVyjKbmxONx\n\t8nrA6u9bKrEddRHr95lK+0HiECf1NwX2LzLmNyYpWBeKI3bKB5t+xySLosXBlCbDUwaR\n\tmFV1bxuHGVity/zVXIi+3/2ZXDsGq9PrC6NTlovnDp3HzgkCJZeqyV6fFBh0Q8xBuCYs\n\tOe6UIkvgjKGTVhXsgVuM9BUwZ63jnyGgKKT77VKb+lRP33jES20XFsJ+J5712xrBJaNN\n\tt8f2nsskm3QMggPzGT4Sa7XGjdT9sDe5kUw/+/3sn87eYXSFFjlqYEWKydA3wb+w2JmY\n\tK63g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=LZt3AiEa0gsDsKn5d8+E3QLwkkRx473N0P3vai9/zlk=;\n\tb=dVtRnpYHPMfSf4wrv9V5u4bi/KZPh/QtOFMOKY6FCIW0guVtnNaosyGkbs2pJ0NHJM\n\twxzOrQmHRYlA0fHMD1KhhIjsHEdYAlXs7HUu5NQAGld6XRpIusiLWRsOKsPWzof/xWST\n\tmTrGnkNno0QQBYWttipS3MUDbZaLdwyAB3gCREo3RXX7XluSffqGxATI54NQWXsxinY6\n\tdiGSKOXMhbRZ+TB9llohNLPuxiUKxuHkXCgdvif7ZWn5U4QnRt+8hWoOhOU9/uTcnx8k\n\tO5c+r0mDNgnpjIO+Qx+65oRL8TIhQKmBt+mkj5FFuaQOIfnX7pOIycR+q24vmctp2hgb\n\tSv3w==","X-Gm-Message-State":"AOAM533l8yOiao2qKW3scyMWAmMz4jXrVJaN98dp6CGolwy31rL41U6q\n\tfv/REnlOHJmL+gqOBB50z0jCZJU4FBMseSdimjpOoA==","X-Google-Smtp-Source":"ABdhPJxA4pTXYdknH5lq8R3LiwyW8KsBlEurDCxjaYkD/JeZfnZBHbFjnE8/HsuicfG3/5pCiWZMBEwDK6uXO37XSC8=","X-Received":"by 2002:a05:600c:17c3:: with SMTP id\n\ty3mr19199452wmo.40.1625566085471; \n\tTue, 06 Jul 2021 03:08:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>","From":"David Plowman ","Date":"Tue, 6 Jul 2021 11:07:54 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18044,"web_url":"https://patchwork.libcamera.org/comment/18044/","msgid":"<3a7ba1b9-2108-284b-ee28-46c8669d150e@ideasonboard.com>","date":"2021-07-09T12:36:13","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\n\nOn 05/07/2021 13:33, Jacopo Mondi wrote:\n> Hi Laurent,\n> \n> On Mon, Jul 05, 2021 at 03:12:45PM +0300, Laurent Pinchart wrote:\n>> Hi Jacopo,\n>>\n>> On Mon, Jul 05, 2021 at 12:48:11PM +0200, Jacopo Mondi wrote:\n>>> On Fri, Jul 02, 2021 at 02:07:41AM +0300, Laurent Pinchart wrote:\n>>>> There are use cases for calling the dispatchMessages() function\n>>>> recursively, from within a message handler. This can be used, for\n>>>> instance, to force delivery of messages posted to a thread concurrently\n>>>> to stopping the thread. This currently causes access, in the outer\n>>>> dispatchMessages() call, to iterators that have been invalidated by\n>>>> erasing list elements in the recursive call, leading to undefined\n>>>> behaviour (most likely double-free or other crashes).\n>>>>\n>>>> Fix it by only erasing messages from the list at the end of the outer\n>>>> call, identified using a recursion counter. We need to keep track of the\n>>>> first non-null entry in the posted messages list to restrict the\n>>>> deletion to the\n\n\nto the .... ?\n\n\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n>>>> Signed-off-by: Laurent Pinchart \n>>>> ---\n>>>> src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n>>>> 1 file changed, 33 insertions(+), 10 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n>>>> index 7f79115222e8..9eb488d46db8 100644\n>>>> --- a/src/libcamera/base/thread.cpp\n>>>> +++ b/src/libcamera/base/thread.cpp\n>>>> @@ -126,6 +126,11 @@ public:\n>>>> \t * \\brief Protects the \\ref list_\n>>>> \t */\n>>>> \tMutex mutex_;\n>>>> +\t/**\n>>>> +\t * \\brief The recursion level for recursive Thread::dispatchMessages()\n>>>> +\t * calls\n>>>> +\t */\n>>>> +\tunsigned int recursion_ = 0;\n>>>> };\n>>>>\n>>>> /**\n>>>> @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n>>>> * Messages shall only be dispatched from the current thread, typically within\n>>>> * the thread from the run() function. Calling this function outside of the\n>>>> * thread results in undefined behaviour.\n>>>> + *\n>>>> + * This function is not thread-safe, but it may be called recursively in the\n>>>> + * same thread from an object's message handler. It guarantees in all cases\n>>>> + * delivery of messages in the order they have been posted.\n\n\"in all cases delivery of\" sounds odd.\n\nPerhaps\n\n\"It guarantees delivery of messages in the order they have been posted\nin all cases\".\n\nOr\n\n\"In all cases, it guarantees delivery of messages in the order they have\nbeen posted\".\n\n\n\n>>>> */\n>>>> void Thread::dispatchMessages(Message::Type type)\n>>>> {\n>>>> \tASSERT(data_ == ThreadData::current());\n>>>>\n>>>> +\t++data_->messages_.recursion_;\n>>>> +\n>>>> \tMutexLocker locker(data_->messages_.mutex_);\n>>>>\n>>>> \tstd::list> &messages = data_->messages_.list_;\n>>>>\n>>>> -\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n>>>> -\t\tstd::unique_ptr &msg = *iter;\n>>>> -\n>>>> -\t\tif (!msg) {\n>>>> -\t\t\titer = data_->messages_.list_.erase(iter);\n>>>> +\tfor (std::unique_ptr &msg : messages) {\n>>>> +\t\tif (!msg)\n>>>> \t\t\tcontinue;\n>>>> -\t\t}\n>>>>\n>>>> -\t\tif (type != Message::Type::None && msg->type() != type) {\n>>>> -\t\t\t++iter;\n>>>> +\t\tif (type != Message::Type::None && msg->type() != type)\n>>>> \t\t\tcontinue;\n>>>> -\t\t}\n>>>>\n>>>> +\t\t/*\n>>>> +\t\t * Move the message, setting the entry in the list to null. It\n>>>> +\t\t * will cause recursive calls to ignore the entry, and the erase\n>>>> +\t\t * loop at the end of the function to delete it from the list.\n>>>> +\t\t */\n>>>> \t\tstd::unique_ptr message = std::move(msg);\n>>>> -\t\titer = data_->messages_.list_.erase(iter);\n>>>>\n>>>> \t\tObject *receiver = message->receiver_;\n>>>> \t\tASSERT(data_ == receiver->thread()->data_);\n>>>> @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n>>>> \t\tmessage.reset();\n>>>> \t\tlocker.lock();\n>>>> \t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * If the recursion level is 0, erase all null messages in the list. We\n>>>> +\t * can't do so during recursion, as it would invalidate the iterator of\n>>>> +\t * the outer calls.\n>>>> +\t */\n>>>\n>>> is it paranoid to think accesses to recursions_ should be serialized ?\n>>\n>> The function is documented as not being thread-safe. This is because it\n>> must be called from the thread created by the Thread class (see the\n>> assert at the beginning that enforces that). While it can be called\n>> recursively, there will never be two concurrent calls, so no race\n>> condition when accessing the recursion_ variable. This is unlike the\n>> messages list that needs to be protected by a lock because other threads\n>> can post messages to the list concurrently to\n>> Thread::dispatchMessages().\n>>\n\nSounds good, and Thread looks well asserted in most cases to guarantee\nrules are adhered to.\n\n\n>>>> +\tif (!--data_->messages_.recursion_) {\n>>>> +\t\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n>>>> +\t\t\tif (!*iter)\n>>>> +\t\t\t\titer = messages.erase(iter);\n>>>> +\t\t\telse\n>>>> +\t\t\t\t++iter;\n>>>> +\t\t}\n>>>> +\t}\n>>>\n>>> Can null messages end up in the list ? can't you just clear() the\n>>> whole list ?\n>>\n>> The function takes a message type parameter to restrict it to\n>> dispatching messages of a given type. The list may thus not be empty, it\n>> may still contain messages of other types.\n> \n> So it is the other way around actually, if the message has been\n> dispatched, we erase it. Sorry, I read the condition wrong, thanks for\n> explaining.\n> \n> A little note, you could:\n> \n> if (--data_->messages_.recursion_)\n> return;\n> \n> And save one indentation level.\n> \n> Apart from this:\n> Reviewed-by: Jacopo Mondi \n\n\nWith comments addressed if and as required:\n\nReviewed-by: Kieran Bingham \n\n\n> \n> Thanks\n> j\n> \n>>\n>>>> }\n>>>>\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 A2ADFC3224\n\tfor ;\n\tFri, 9 Jul 2021 12:36:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECBE96851B;\n\tFri, 9 Jul 2021 14:36:17 +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 CECA0605AC\n\tfor ;\n\tFri, 9 Jul 2021 14:36:16 +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 4178FE7;\n\tFri, 9 Jul 2021 14:36:16 +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=\"q9eAj6Pm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625834176;\n\tbh=hVv4AtSLuvHBDvg8DWSD5FRM13ZAAKEUrMBd44mHXh0=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=q9eAj6Pmh8IE+uNxl8z1riSUg5EkAqbe95Ug4gPJeXEFmmnY0mTMuS2UXlmAw1FQA\n\tpMdgsOVTzb897M96tWLIEoeAebVPxDdArkKVQK7hhcZy0hdjqBynnYIOHqURnZGC1m\n\toEQaD31PAHSg2VqjNz3YHzDo00vdDlPlm42s16ts=","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>\n\t<20210705104811.eguavrps6mflgcnr@uno.localdomain>\n\t\n\t<20210705123303.ncen4quzugvyzmxa@uno.localdomain>","From":"Kieran Bingham ","Message-ID":"<3a7ba1b9-2108-284b-ee28-46c8669d150e@ideasonboard.com>","Date":"Fri, 9 Jul 2021 13:36:13 +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":"<20210705123303.ncen4quzugvyzmxa@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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":18073,"web_url":"https://patchwork.libcamera.org/comment/18073/","msgid":"","date":"2021-07-11T14:27:47","subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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 01:36:13PM +0100, Kieran Bingham wrote:\n> On 05/07/2021 13:33, Jacopo Mondi wrote:\n> > On Mon, Jul 05, 2021 at 03:12:45PM +0300, Laurent Pinchart wrote:\n> >> On Mon, Jul 05, 2021 at 12:48:11PM +0200, Jacopo Mondi wrote:\n> >>> On Fri, Jul 02, 2021 at 02:07:41AM +0300, Laurent Pinchart wrote:\n> >>>> There are use cases for calling the dispatchMessages() function\n> >>>> recursively, from within a message handler. This can be used, for\n> >>>> instance, to force delivery of messages posted to a thread concurrently\n> >>>> to stopping the thread. This currently causes access, in the outer\n> >>>> dispatchMessages() call, to iterators that have been invalidated by\n> >>>> erasing list elements in the recursive call, leading to undefined\n> >>>> behaviour (most likely double-free or other crashes).\n> >>>>\n> >>>> Fix it by only erasing messages from the list at the end of the outer\n> >>>> call, identified using a recursion counter. We need to keep track of the\n> >>>> first non-null entry in the posted messages list to restrict the\n> >>>> deletion to the\n> \n> to the .... ?\n\n:-)\n\nI'll drop the whole sentence, it's a leftover of a previous more complex\nimplementation, the current version doesn't keep track of the first\nnon-null entry.\n\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=26\n> >>>> Signed-off-by: Laurent Pinchart \n> >>>> ---\n> >>>> src/libcamera/base/thread.cpp | 43 +++++++++++++++++++++++++++--------\n> >>>> 1 file changed, 33 insertions(+), 10 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> >>>> index 7f79115222e8..9eb488d46db8 100644\n> >>>> --- a/src/libcamera/base/thread.cpp\n> >>>> +++ b/src/libcamera/base/thread.cpp\n> >>>> @@ -126,6 +126,11 @@ public:\n> >>>> \t * \\brief Protects the \\ref list_\n> >>>> \t */\n> >>>> \tMutex mutex_;\n> >>>> +\t/**\n> >>>> +\t * \\brief The recursion level for recursive Thread::dispatchMessages()\n> >>>> +\t * calls\n> >>>> +\t */\n> >>>> +\tunsigned int recursion_ = 0;\n> >>>> };\n> >>>>\n> >>>> /**\n> >>>> @@ -595,30 +600,34 @@ void Thread::removeMessages(Object *receiver)\n> >>>> * Messages shall only be dispatched from the current thread, typically within\n> >>>> * the thread from the run() function. Calling this function outside of the\n> >>>> * thread results in undefined behaviour.\n> >>>> + *\n> >>>> + * This function is not thread-safe, but it may be called recursively in the\n> >>>> + * same thread from an object's message handler. It guarantees in all cases\n> >>>> + * delivery of messages in the order they have been posted.\n> \n> \"in all cases delivery of\" sounds odd.\n> \n> Perhaps\n> \n> \"It guarantees delivery of messages in the order they have been posted\n> in all cases\".\n\nI'll use this wording.\n\n> Or\n> \n> \"In all cases, it guarantees delivery of messages in the order they have\n> been posted\".\n> \n> >>>> */\n> >>>> void Thread::dispatchMessages(Message::Type type)\n> >>>> {\n> >>>> \tASSERT(data_ == ThreadData::current());\n> >>>>\n> >>>> +\t++data_->messages_.recursion_;\n> >>>> +\n> >>>> \tMutexLocker locker(data_->messages_.mutex_);\n> >>>>\n> >>>> \tstd::list> &messages = data_->messages_.list_;\n> >>>>\n> >>>> -\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> >>>> -\t\tstd::unique_ptr &msg = *iter;\n> >>>> -\n> >>>> -\t\tif (!msg) {\n> >>>> -\t\t\titer = data_->messages_.list_.erase(iter);\n> >>>> +\tfor (std::unique_ptr &msg : messages) {\n> >>>> +\t\tif (!msg)\n> >>>> \t\t\tcontinue;\n> >>>> -\t\t}\n> >>>>\n> >>>> -\t\tif (type != Message::Type::None && msg->type() != type) {\n> >>>> -\t\t\t++iter;\n> >>>> +\t\tif (type != Message::Type::None && msg->type() != type)\n> >>>> \t\t\tcontinue;\n> >>>> -\t\t}\n> >>>>\n> >>>> +\t\t/*\n> >>>> +\t\t * Move the message, setting the entry in the list to null. It\n> >>>> +\t\t * will cause recursive calls to ignore the entry, and the erase\n> >>>> +\t\t * loop at the end of the function to delete it from the list.\n> >>>> +\t\t */\n> >>>> \t\tstd::unique_ptr message = std::move(msg);\n> >>>> -\t\titer = data_->messages_.list_.erase(iter);\n> >>>>\n> >>>> \t\tObject *receiver = message->receiver_;\n> >>>> \t\tASSERT(data_ == receiver->thread()->data_);\n> >>>> @@ -629,6 +638,20 @@ void Thread::dispatchMessages(Message::Type type)\n> >>>> \t\tmessage.reset();\n> >>>> \t\tlocker.lock();\n> >>>> \t}\n> >>>> +\n> >>>> +\t/*\n> >>>> +\t * If the recursion level is 0, erase all null messages in the list. We\n> >>>> +\t * can't do so during recursion, as it would invalidate the iterator of\n> >>>> +\t * the outer calls.\n> >>>> +\t */\n> >>>\n> >>> is it paranoid to think accesses to recursions_ should be serialized ?\n> >>\n> >> The function is documented as not being thread-safe. This is because it\n> >> must be called from the thread created by the Thread class (see the\n> >> assert at the beginning that enforces that). While it can be called\n> >> recursively, there will never be two concurrent calls, so no race\n> >> condition when accessing the recursion_ variable. This is unlike the\n> >> messages list that needs to be protected by a lock because other threads\n> >> can post messages to the list concurrently to\n> >> Thread::dispatchMessages().\n> \n> Sounds good, and Thread looks well asserted in most cases to guarantee\n> rules are adhered to.\n> \n> >>>> +\tif (!--data_->messages_.recursion_) {\n> >>>> +\t\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n> >>>> +\t\t\tif (!*iter)\n> >>>> +\t\t\t\titer = messages.erase(iter);\n> >>>> +\t\t\telse\n> >>>> +\t\t\t\t++iter;\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>\n> >>> Can null messages end up in the list ? can't you just clear() the\n> >>> whole list ?\n> >>\n> >> The function takes a message type parameter to restrict it to\n> >> dispatching messages of a given type. The list may thus not be empty, it\n> >> may still contain messages of other types.\n> > \n> > So it is the other way around actually, if the message has been\n> > dispatched, we erase it. Sorry, I read the condition wrong, thanks for\n> > explaining.\n> > \n> > A little note, you could:\n> > \n> > if (--data_->messages_.recursion_)\n> > return;\n> > \n> > And save one indentation level.\n> > \n> > Apart from this:\n> > Reviewed-by: Jacopo Mondi \n> \n> With comments addressed if and as required:\n> \n> Reviewed-by: Kieran Bingham \n> \n> >>>> }\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 95890BD794\n\tfor ;\n\tSun, 11 Jul 2021 14:28:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0842E6851D;\n\tSun, 11 Jul 2021 16:28:35 +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 0A07C68519\n\tfor ;\n\tSun, 11 Jul 2021 16:28:33 +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 6EF70CC;\n\tSun, 11 Jul 2021 16:28:32 +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=\"IkDK5TlN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626013712;\n\tbh=Sk7GP2eETwAPnKIylXUtipsaCu2+QNTWLJ+SKIqjKPw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IkDK5TlNvgoziv1UeIEPpO55bCrxa+PYqTpx7Iu74/3zIVp8h/aw21qz4ZtQC8Lhw\n\twMArFnfCCHb8wMmZFyA9CeNfAtV4Vcm0lZETfm+jGCzUeB5ifXRKcOX0o4fw01puvw\n\t93DdYCqgN8fr4R5WvrGhl9e6wiP33dylIWTxgR8M=","Date":"Sun, 11 Jul 2021 17:27:47 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210701230741.14320-1-laurent.pinchart@ideasonboard.com>\n\t<20210701230741.14320-4-laurent.pinchart@ideasonboard.com>\n\t<20210705104811.eguavrps6mflgcnr@uno.localdomain>\n\t\n\t<20210705123303.ncen4quzugvyzmxa@uno.localdomain>\n\t<3a7ba1b9-2108-284b-ee28-46c8669d150e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3a7ba1b9-2108-284b-ee28-46c8669d150e@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] base: thread: Fix recursive calls\n\tto dispatchMessages()","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\" "}}]