[{"id":3528,"web_url":"https://patchwork.libcamera.org/comment/3528/","msgid":"<20200120113020.nwstq6sjpktxqphj@uno.localdomain>","date":"2020-01-20T11:30:20","subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 20, 2020 at 02:24:20AM +0200, Laurent Pinchart wrote:\n> ConnectionTypeBlocking always invokes the method through inter-thread\n> message passing, which results in deadlocks if the sender and receiver\n> live in the same thread. The deadlock can easily be avoided by turning\n> the invocation into a direct call in this case. Do so to make\n> ConnectionTypeBlocking easier to use when some of the senders live in\n> the same thread as the receiver while the other senders don't.\n>\n> Extend the object-invoke test to cover this usage.\n>\n> While at it reformat the documentation to avoid long \\brief lines.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> src/libcamera/bound_method.cpp | 22 ++++++++++++++--------\n> test/object-invoke.cpp | 20 ++++++++++++++++++++\n> 2 files changed, 34 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp\n> index e18c2eb4c68e..9aa59dc3678f 100644\n> --- a/src/libcamera/bound_method.cpp\n> +++ b/src/libcamera/bound_method.cpp\n> @@ -35,16 +35,19 @@ namespace libcamera {\n> * thread.\n> *\n> * \\var ConnectionType::ConnectionTypeQueued\n> - * \\brief The receiver is invoked asynchronously in its thread when control\n> - * returns to the thread's event loop. The sender proceeds without waiting for\n> - * the invocation to complete.\n> + * \\brief The receiver is invoked asynchronously\n> + *\n> + * Invoke the receiver asynchronously in its thread when control returns to the\n> + * thread's event loop. The sender proceeds without waiting for the invocation\n> + * to complete.\n\nI find this a bit terse, but it might just be me.\n\nInvoke the receiver asynchronously. The called method or activated\nslot are executed once the control returns to the receiver's thread\nevent loop. The caller proceeds without waiting for the\ninvocation to complete.\n\n> *\n> * \\var ConnectionType::ConnectionTypeBlocking\n> - * \\brief The receiver is invoked asynchronously in its thread when control\n> - * returns to the thread's event loop. The sender blocks until the receiver\n> - * signals the completion of the invocation. This connection type shall not be\n> - * used when the sender and receiver live in the same thread, otherwise\n> - * deadlock will occur.\n> + * \\brief The receiver is invoked synchronously\n> + *\n> + * If the sender and the receiver live in the same thread, this is equivalent to\n\nI would move this new part to the end.\n\n> + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in\n> + * its thread when control returns to the thread's event loop. The sender\n> + * blocks until the receiver signals the completion of the invocation.\n\nInvoke the receiver asynchronously. The called method or activated\nslot are executed once the control returns to the receiver's thread\nevent loop. The sender block until the receiver ...\nIf the sender and the receiver live in the same thread...\n\n> */\n>\n> /**\n> @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr pack,\n> \t\t\ttype = ConnectionTypeDirect;\n> \t\telse\n> \t\t\ttype = ConnectionTypeQueued;\n> +\t} else if (type == ConnectionTypeBlocking) {\n> +\t\tif (Thread::current() == object_->thread())\n\nOr\n } else if (type == ConnectionTypeBlocking &&\n\t\t Thread::current() == object_->thread())\n type = ConnectionTypeDirect;\n\n> +\t\t\ttype = ConnectionTypeDirect;\n> \t}\n>\n> \tswitch (type) {\n> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n> index 8e2055ca620f..fa162c838c78 100644\n> --- a/test/object-invoke.cpp\n> +++ b/test/object-invoke.cpp\n> @@ -100,6 +100,26 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> +\t\t/*\n> +\t\t * Test that blocking invocation is delivered directly when the\n> +\t\t * caller and callee live in the same thread.\n> +\t\t */\n> +\t\tobject_.reset();\n> +\n> +\t\tobject_.invokeMethod(&InvokedObject::method,\n> +\t\t\t\t ConnectionTypeBlocking, 42);\n> +\n> +\t\tswitch (object_.status()) {\n> +\t\tcase InvokedObject::NoCall:\n> +\t\t\tcout << \"Method not invoked for main thread (blocking)\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\tcase InvokedObject::InvalidThread:\n> +\t\t\tcout << \"Method invoked in incorrect thread for main thread (blocking)\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\tdefault:\n> +\t\t\tbreak;\n> +\t\t}\n> +\n\nYou might want to be paranoid and check the value as its done in the\nother method invocations in the test\n\n\t\tif (object_.value() != 42) {\n\t\t\tcout << \"Method invoked with incorrect value for custom thread\" << endl;\n\t\t\treturn TestFail;\n\t\t}\n\n> \t\t/*\n> \t\t * Move the object to a thread and verify that auto method\n> \t\t * invocation is delivered in the correct thread.\n\nAll minors though\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0EFCF600F5\n\tfor ;\n\tMon, 20 Jan 2020 12:27:49 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 936C91C0003;\n\tMon, 20 Jan 2020 11:27:48 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Mon, 20 Jan 2020 12:30:20 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120113020.nwstq6sjpktxqphj@uno.localdomain>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"iykihj63ok7ctmja\"","Content-Disposition":"inline","In-Reply-To":"<20200120002437.6633-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","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","X-List-Received-Date":"Mon, 20 Jan 2020 11:27:49 -0000"}},{"id":3531,"web_url":"https://patchwork.libcamera.org/comment/3531/","msgid":"<20200120114337.GD20122@pendragon.ideasonboard.com>","date":"2020-01-20T11:43:37","subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","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, Jan 20, 2020 at 12:30:20PM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 20, 2020 at 02:24:20AM +0200, Laurent Pinchart wrote:\n> > ConnectionTypeBlocking always invokes the method through inter-thread\n> > message passing, which results in deadlocks if the sender and receiver\n> > live in the same thread. The deadlock can easily be avoided by turning\n> > the invocation into a direct call in this case. Do so to make\n> > ConnectionTypeBlocking easier to use when some of the senders live in\n> > the same thread as the receiver while the other senders don't.\n> >\n> > Extend the object-invoke test to cover this usage.\n> >\n> > While at it reformat the documentation to avoid long \\brief lines.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > src/libcamera/bound_method.cpp | 22 ++++++++++++++--------\n> > test/object-invoke.cpp | 20 ++++++++++++++++++++\n> > 2 files changed, 34 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp\n> > index e18c2eb4c68e..9aa59dc3678f 100644\n> > --- a/src/libcamera/bound_method.cpp\n> > +++ b/src/libcamera/bound_method.cpp\n> > @@ -35,16 +35,19 @@ namespace libcamera {\n> > * thread.\n> > *\n> > * \\var ConnectionType::ConnectionTypeQueued\n> > - * \\brief The receiver is invoked asynchronously in its thread when control\n> > - * returns to the thread's event loop. The sender proceeds without waiting for\n> > - * the invocation to complete.\n> > + * \\brief The receiver is invoked asynchronously\n> > + *\n> > + * Invoke the receiver asynchronously in its thread when control returns to the\n> > + * thread's event loop. The sender proceeds without waiting for the invocation\n> > + * to complete.\n> \n> I find this a bit terse, but it might just be me.\n\nNote that this isn't introduced by this patch, just reformatted.\n\n> Invoke the receiver asynchronously.\n\nThis duplicates the brief :-)\n\n> The called method or activated\n> slot are executed once the control returns to the receiver's thread\n\nI wouldn't mention slots here to keep the documentation agnostic of the\nupper layers.\n\n> event loop. The caller proceeds without waiting for the\n> invocation to complete.\n\nI'm not sure this would be an improvement, but it might just be me :-)\n\n> > *\n> > * \\var ConnectionType::ConnectionTypeBlocking\n> > - * \\brief The receiver is invoked asynchronously in its thread when control\n> > - * returns to the thread's event loop. The sender blocks until the receiver\n> > - * signals the completion of the invocation. This connection type shall not be\n> > - * used when the sender and receiver live in the same thread, otherwise\n> > - * deadlock will occur.\n> > + * \\brief The receiver is invoked synchronously\n> > + *\n> > + * If the sender and the receiver live in the same thread, this is equivalent to\n> \n> I would move this new part to the end.\n> \n> > + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in\n> > + * its thread when control returns to the thread's event loop. The sender\n> > + * blocks until the receiver signals the completion of the invocation.\n> \n> Invoke the receiver asynchronously. The called method or activated\n> slot are executed once the control returns to the receiver's thread\n> event loop. The sender block until the receiver ...\n> If the sender and the receiver live in the same thread...\n\nIt would then make the beginning of the paragraph uncorrect :-) It could\nbe written\n\n\"If the sender and the receiver live in different threads, invoke the\nreceiver ... Otherwise, this is equivalent to ConnectionTypeDirect.\"\n\nDo you think that would be better ?\n\n> > */\n> >\n> > /**\n> > @@ -71,6 +74,9 @@ bool BoundMethodBase::activatePack(std::shared_ptr pack,\n> > \t\t\ttype = ConnectionTypeDirect;\n> > \t\telse\n> > \t\t\ttype = ConnectionTypeQueued;\n> > +\t} else if (type == ConnectionTypeBlocking) {\n> > +\t\tif (Thread::current() == object_->thread())\n> \n> Or\n> } else if (type == ConnectionTypeBlocking &&\n> \t\t Thread::current() == object_->thread())\n> type = ConnectionTypeDirect;\n\nI like the existing syntax better, I think it's clearer.\n\n> > +\t\t\ttype = ConnectionTypeDirect;\n> > \t}\n> >\n> > \tswitch (type) {\n> > diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n> > index 8e2055ca620f..fa162c838c78 100644\n> > --- a/test/object-invoke.cpp\n> > +++ b/test/object-invoke.cpp\n> > @@ -100,6 +100,26 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > +\t\t/*\n> > +\t\t * Test that blocking invocation is delivered directly when the\n> > +\t\t * caller and callee live in the same thread.\n> > +\t\t */\n> > +\t\tobject_.reset();\n> > +\n> > +\t\tobject_.invokeMethod(&InvokedObject::method,\n> > +\t\t\t\t ConnectionTypeBlocking, 42);\n> > +\n> > +\t\tswitch (object_.status()) {\n> > +\t\tcase InvokedObject::NoCall:\n> > +\t\t\tcout << \"Method not invoked for main thread (blocking)\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\tcase InvokedObject::InvalidThread:\n> > +\t\t\tcout << \"Method invoked in incorrect thread for main thread (blocking)\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\tdefault:\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> \n> You might want to be paranoid and check the value as its done in the\n> other method invocations in the test\n> \n> \t\tif (object_.value() != 42) {\n> \t\t\tcout << \"Method invoked with incorrect value for custom thread\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n\nI've omitted that specifically because the value propagation until the\nreceiver in the direct connection case (which ConnectionTypeBlocking\nresolves to in this case) is already tested elsewhere.\n\n> > \t\t/*\n> > \t\t * Move the object to a thread and verify that auto method\n> > \t\t * invocation is delivered in the correct thread.\n> \n> All minors though\n> Reviewed-by: Jacopo Mondi ","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FF0A600F5\n\tfor ;\n\tMon, 20 Jan 2020 12:43:38 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6A0CA62;\n\tMon, 20 Jan 2020 12:43:37 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579520617;\n\tbh=KL6pf4T7D6ATFR5abdeYmFm+DEy7xlCOG6ioUPJCPNY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QGsQNcONqmIxiAfF5v3Gn1Mx0kz4QA99eITghDM18EtajN8IaUaJ1c+bi7g602Pty\n\tu9obJzVxL/dr97Kg6KoFT70chAgmtm9HTRIvTaMstuQGznZoZihj0jz3zeBLigNkNA\n\t3gJcn0MC7ZjWwlPna12niiK6Dyr1rqdPcsdPdDyM=","Date":"Mon, 20 Jan 2020 13:43:37 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120114337.GD20122@pendragon.ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-3-laurent.pinchart@ideasonboard.com>\n\t<20200120113020.nwstq6sjpktxqphj@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200120113020.nwstq6sjpktxqphj@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","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","X-List-Received-Date":"Mon, 20 Jan 2020 11:43:38 -0000"}},{"id":3534,"web_url":"https://patchwork.libcamera.org/comment/3534/","msgid":"<20200120124658.hpcjwo5zbkis5jz6@uno.localdomain>","date":"2020-01-20T12:46:58","subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Jan 20, 2020 at 01:43:37PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jan 20, 2020 at 12:30:20PM +0100, Jacopo Mondi wrote:\n> > On Mon, Jan 20, 2020 at 02:24:20AM +0200, Laurent Pinchart wrote:\n> > > ConnectionTypeBlocking always invokes the method through inter-thread\n> > > message passing, which results in deadlocks if the sender and receiver\n> > > live in the same thread. The deadlock can easily be avoided by turning\n> > > the invocation into a direct call in this case. Do so to make\n> > > ConnectionTypeBlocking easier to use when some of the senders live in\n> > > the same thread as the receiver while the other senders don't.\n> > >\n> > > Extend the object-invoke test to cover this usage.\n> > >\n> > > While at it reformat the documentation to avoid long \\brief lines.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/bound_method.cpp | 22 ++++++++++++++--------\n> > > test/object-invoke.cpp | 20 ++++++++++++++++++++\n> > > 2 files changed, 34 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp\n> > > index e18c2eb4c68e..9aa59dc3678f 100644\n> > > --- a/src/libcamera/bound_method.cpp\n> > > +++ b/src/libcamera/bound_method.cpp\n> > > @@ -35,16 +35,19 @@ namespace libcamera {\n> > > * thread.\n> > > *\n> > > * \\var ConnectionType::ConnectionTypeQueued\n> > > - * \\brief The receiver is invoked asynchronously in its thread when control\n> > > - * returns to the thread's event loop. The sender proceeds without waiting for\n> > > - * the invocation to complete.\n> > > + * \\brief The receiver is invoked asynchronously\n> > > + *\n> > > + * Invoke the receiver asynchronously in its thread when control returns to the\n> > > + * thread's event loop. The sender proceeds without waiting for the invocation\n> > > + * to complete.\n> >\n> > I find this a bit terse, but it might just be me.\n>\n> Note that this isn't introduced by this patch, just reformatted.\n>\n> > Invoke the receiver asynchronously.\n>\n> This duplicates the brief :-)\n>\n> > The called method or activated\n> > slot are executed once the control returns to the receiver's thread\n>\n> I wouldn't mention slots here to keep the documentation agnostic of the\n> upper layers.\n>\n> > event loop. The caller proceeds without waiting for the\n> > invocation to complete.\n>\n> I'm not sure this would be an improvement, but it might just be me :-)\n>\n> > > *\n> > > * \\var ConnectionType::ConnectionTypeBlocking\n> > > - * \\brief The receiver is invoked asynchronously in its thread when control\n> > > - * returns to the thread's event loop. The sender blocks until the receiver\n> > > - * signals the completion of the invocation. This connection type shall not be\n> > > - * used when the sender and receiver live in the same thread, otherwise\n> > > - * deadlock will occur.\n> > > + * \\brief The receiver is invoked synchronously\n> > > + *\n> > > + * If the sender and the receiver live in the same thread, this is equivalent to\n> >\n> > I would move this new part to the end.\n> >\n> > > + * ConnectionTypeDirect. Otherwise, the receiver is invoked asynchronously in\n> > > + * its thread when control returns to the thread's event loop. The sender\n> > > + * blocks until the receiver signals the completion of the invocation.\n> >\n> > Invoke the receiver asynchronously. The called method or activated\n> > slot are executed once the control returns to the receiver's thread\n> > event loop. The sender block until the receiver ...\n> > If the sender and the receiver live in the same thread...\n>\n> It would then make the beginning of the paragraph uncorrect :-) It could\n> be written\n>\n> \"If the sender and the receiver live in different threads, invoke the\n> receiver ... Otherwise, this is equivalent to ConnectionTypeDirect.\"\n>\n> Do you think that would be better ?\n>\n\n\nWhatever is fine, take what you consider more opportune in\n> >\n> > All minors though\n> > Reviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71FD4600F5\n\tfor ;\n\tMon, 20 Jan 2020 13:44:27 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 04B3920000B;\n\tMon, 20 Jan 2020 12:44:26 +0000 (UTC)"],"Date":"Mon, 20 Jan 2020 13:46:58 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120124658.hpcjwo5zbkis5jz6@uno.localdomain>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-3-laurent.pinchart@ideasonboard.com>\n\t<20200120113020.nwstq6sjpktxqphj@uno.localdomain>\n\t<20200120114337.GD20122@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"thrqzkdpzxo77kfz\"","Content-Disposition":"inline","In-Reply-To":"<20200120114337.GD20122@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 02/19] libcamera: bound_method: Avoid\n\tdeadlock with ConnectionTypeBlocking","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","X-List-Received-Date":"Mon, 20 Jan 2020 12:44:27 -0000"}}]