[{"id":33285,"web_url":"https://patchwork.libcamera.org/comment/33285/","msgid":"<3umsku7dm5t6gv3wcevawolpemayohansgfsbzcb34cseabcjg@xqakjpm5elb6>","date":"2025-02-04T13:34:55","subject":"Re: [RFC PATCH v3 07/21] apps: common: event_loop: Make it possible\n\tto exit with exception","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jan 30, 2025 at 11:50:36AM +0000, Barnabás Pőcze wrote:\n> Instead of taking a single integer in `EventLoop:exit()`, change it\n> so that an `std::exception_ptr` can also be passed. And in `EventLoop::exec()`\n> rethrow the stored exception (if any).\n>\n> Furthermore, catch exceptions from deferred calls, and stop the event\n> loop with the caught exception.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/common/event_loop.cpp | 35 +++++++++++++++++++++++++++++-----\n> src/apps/common/event_loop.h | 6 ++++--\n> 2 files changed, 34 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp\n> index 0d7a4a024..13c2d6057 100644\n> --- a/src/apps/common/event_loop.cpp\n> +++ b/src/apps/common/event_loop.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> EventLoop *EventLoop::instance_ = nullptr;\n>\n> @@ -38,7 +39,14 @@ EventLoop::EventLoop()\n> \t\t\t\tself->calls_.pop_front();\n> \t\t\t}\n>\n> -\t\t\tcall();\n> +\t\t\ttry {\n> +\t\t\t\tcall();\n> +\t\t\t}\n> +\t\t\tcatch (...) {\n\nwe don't have a preferred syntax for try {} catch () {}\nbut I feel like it should probably be\n\n try {\n\n } catch () {\n\n }\n\nto match how we handle if() branches ?\n\n> +\t\t\t\t::event_active(self->callsTrigger_, 0, 0);\n> +\t\t\t\tself->exit(std::current_exception());\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \t\t}\n> \t}, this);\n> \tassert(callsTrigger_);\n> @@ -63,14 +71,31 @@ EventLoop *EventLoop::instance()\n>\n> int EventLoop::exec()\n> {\n> -\texitCode_ = -1;\n> +\t{\n> +\t\tstd::lock_guard locker(lock_);\n> +\t\tresult_ = 0;\n> +\t}\n> +\n> \tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> -\treturn exitCode_;\n> +\n> +\tauto result = [&] {\n> +\t\tstd::lock_guard locker(lock_);\n> +\t\treturn std::exchange(result_, 0);\n> +\t}();\n> +\n> +\tif (auto *exc = std::get_if(&result))\n> +\t\tstd::rethrow_exception(std::move(*exc));\n> +\n> +\treturn std::get(result);\n\nAll of this is morbidly fascinating as only C++ can be.\n\nI understand this whole patch is only needed to support the following\nhunk in 19/21\n\n- if (queueRequest(request))\n- loop_->exit(-EINVAL);\n+\tqueueRequest(request);\n\niow to remove the need for explicit loop interruptions but to\npropagate the exceptions thrown by gtest ASSERT_EQ() and friends in\nthe even loop and exit there.\n\nAll of this is neat but I wonder if the complexity of this patch is\nnecessary just to support that single use case. Maybe it's complex for\nme but not for others, I'll ask their opinion.\n\n> }\n>\n> -void EventLoop::exit(int code)\n> +void EventLoop::exit(std::variant result)\n> {\n> -\texitCode_ = code;\n> +\t{\n> +\t\tstd::lock_guard locker(lock_);\n> +\t\tresult_ = std::move(result);\n> +\t}\n> +\n> \tevent_base_loopbreak(base_);\n> }\n>\n> diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h\n> index 6d7d0497a..380c3483a 100644\n> --- a/src/apps/common/event_loop.h\n> +++ b/src/apps/common/event_loop.h\n> @@ -9,11 +9,13 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> #include \n> +#include \n>\n> #include \n>\n> @@ -35,7 +37,7 @@ public:\n> \tstatic EventLoop *instance();\n>\n> \tint exec();\n> -\tvoid exit(int code = 0);\n> +\tvoid exit(std::variant result = 0);\n>\n> \tvoid callLater(std::function &&func, std::optional cookie = {});\n> \tvoid cancelLater(std::optional cookie = {});\n> @@ -63,7 +65,7 @@ private:\n> \tstatic EventLoop *instance_;\n>\n> \tstruct event_base *base_;\n> -\tint exitCode_;\n> +\tstd::variant result_ = 0;\n>\n> \tstd::deque, std::optional>> calls_;\n> \t::event *callsTrigger_ = nullptr;\n> --\n> 2.48.1\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 15644C32F1\n\tfor ;\n\tTue, 4 Feb 2025 13:35:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB93D685AC;\n\tTue, 4 Feb 2025 14:35:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E63A6053F\n\tfor ;\n\tTue, 4 Feb 2025 14:34:59 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2FC4CDB;\n\tTue, 4 Feb 2025 14:33:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OkaP/hDL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738676027;\n\tbh=SOl99HkdnBey3MAdbFgVHXDs0G+AWdASIKd4c1mqjwc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OkaP/hDLDor+i4O0SvA4hhCA0jo0XNJve8+50YruDOvwrof8FiVFVioWWZBxCthY0\n\tYOTZdYoR9h7uz6etibhmW6Y6oIddq0+cx5pPiRme8/i7yeU6sVAPxe9ORt0j0ZdJQM\n\t74dj0cpdAAuWIS9VjiYR9KPKccfFgzXg1uu+kb5w=","Date":"Tue, 4 Feb 2025 14:34:55 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 07/21] apps: common: event_loop: Make it possible\n\tto exit with exception","Message-ID":"<3umsku7dm5t6gv3wcevawolpemayohansgfsbzcb34cseabcjg@xqakjpm5elb6>","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-8-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250130115001.1129305-8-pobrn@protonmail.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33384,"web_url":"https://patchwork.libcamera.org/comment/33384/","msgid":"","date":"2025-02-17T14:43:35","subject":"Re: [RFC PATCH v3 07/21] apps: common: event_loop: Make it possible\n\tto exit with exception","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 4., kedd 14:34 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Thu, Jan 30, 2025 at 11:50:36AM +0000, Barnabás Pőcze wrote:\n> > Instead of taking a single integer in `EventLoop:exit()`, change it\n> > so that an `std::exception_ptr` can also be passed. And in `EventLoop::exec()`\n> > rethrow the stored exception (if any).\n> >\n> > Furthermore, catch exceptions from deferred calls, and stop the event\n> > loop with the caught exception.\n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/apps/common/event_loop.cpp | 35 +++++++++++++++++++++++++++++-----\n> > src/apps/common/event_loop.h | 6 ++++--\n> > 2 files changed, 34 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp\n> > index 0d7a4a024..13c2d6057 100644\n> > --- a/src/apps/common/event_loop.cpp\n> > +++ b/src/apps/common/event_loop.cpp\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > EventLoop *EventLoop::instance_ = nullptr;\n> >\n> > @@ -38,7 +39,14 @@ EventLoop::EventLoop()\n> > \t\t\t\tself->calls_.pop_front();\n> > \t\t\t}\n> >\n> > -\t\t\tcall();\n> > +\t\t\ttry {\n> > +\t\t\t\tcall();\n> > +\t\t\t}\n> > +\t\t\tcatch (...) {\n> \n> we don't have a preferred syntax for try {} catch () {}\n> but I feel like it should probably be\n> \n> try {\n> \n> } catch () {\n> \n> }\n> \n> to match how we handle if() branches ?\n> \n> > +\t\t\t\t::event_active(self->callsTrigger_, 0, 0);\n> > +\t\t\t\tself->exit(std::current_exception());\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > \t\t}\n> > \t}, this);\n> > \tassert(callsTrigger_);\n> > @@ -63,14 +71,31 @@ EventLoop *EventLoop::instance()\n> >\n> > int EventLoop::exec()\n> > {\n> > -\texitCode_ = -1;\n> > +\t{\n> > +\t\tstd::lock_guard locker(lock_);\n> > +\t\tresult_ = 0;\n> > +\t}\n> > +\n> > \tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > -\treturn exitCode_;\n> > +\n> > +\tauto result = [&] {\n> > +\t\tstd::lock_guard locker(lock_);\n> > +\t\treturn std::exchange(result_, 0);\n> > +\t}();\n> > +\n> > +\tif (auto *exc = std::get_if(&result))\n> > +\t\tstd::rethrow_exception(std::move(*exc));\n> > +\n> > +\treturn std::get(result);\n> \n> All of this is morbidly fascinating as only C++ can be.\n> \n> I understand this whole patch is only needed to support the following\n> hunk in 19/21\n> \n> - if (queueRequest(request))\n> - loop_->exit(-EINVAL);\n> +\tqueueRequest(request);\n> \n> iow to remove the need for explicit loop interruptions but to\n> propagate the exceptions thrown by gtest ASSERT_EQ() and friends in\n> the even loop and exit there.\n> \n> All of this is neat but I wonder if the complexity of this patch is\n> necessary just to support that single use case. Maybe it's complex for\n> me but not for others, I'll ask their opinion.\n\nI think it's the cleanest solution. One does not have to always wonder in which\ncontext the code is run, and how an error can be propagated from that context.\nAnd a gtest exception contains much more information than a nondescript integer.\n\n---\n\n`-fno-exceptions` is enabled when compiling for chromeos, so this does not compile\nthere. I can see two solutions for that:\n\n * add `-fexceptions` somewhere that includes event_loop.cpp\n * use `#if __cpp_exceptions` and remove the try-catch block accordingly\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > }\n> >\n> > -void EventLoop::exit(int code)\n> > +void EventLoop::exit(std::variant result)\n> > {\n> > -\texitCode_ = code;\n> > +\t{\n> > +\t\tstd::lock_guard locker(lock_);\n> > +\t\tresult_ = std::move(result);\n> > +\t}\n> > +\n> > \tevent_base_loopbreak(base_);\n> > }\n> >\n> > diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h\n> > index 6d7d0497a..380c3483a 100644\n> > --- a/src/apps/common/event_loop.h\n> > +++ b/src/apps/common/event_loop.h\n> > @@ -9,11 +9,13 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -35,7 +37,7 @@ public:\n> > \tstatic EventLoop *instance();\n> >\n> > \tint exec();\n> > -\tvoid exit(int code = 0);\n> > +\tvoid exit(std::variant result = 0);\n> >\n> > \tvoid callLater(std::function &&func, std::optional cookie = {});\n> > \tvoid cancelLater(std::optional cookie = {});\n> > @@ -63,7 +65,7 @@ private:\n> > \tstatic EventLoop *instance_;\n> >\n> > \tstruct event_base *base_;\n> > -\tint exitCode_;\n> > +\tstd::variant result_ = 0;\n> >\n> > \tstd::deque, std::optional>> calls_;\n> > \t::event *callsTrigger_ = nullptr;\n> > --\n> > 2.48.1\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 ECA9CBDE6B\n\tfor ;\n\tMon, 17 Feb 2025 14:43:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BDFB6866D;\n\tMon, 17 Feb 2025 15:43:43 +0100 (CET)","from mail-10629.protonmail.ch (mail-10629.protonmail.ch\n\t[79.135.106.29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D808D61865\n\tfor ;\n\tMon, 17 Feb 2025 15:43:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"rsWn036b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739803419; x=1740062619;\n\tbh=VsVBWL65nZG84F1HW3Asz2UpSdViv7neoSeBdQ+ZZdM=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=rsWn036bJbdW+FrYSdkSsbBtcBfy+5YTl23rLYcq5G16S/HWfUSxRc+SAlBIkzIeJ\n\tz0W/YdrfpHUhfx8lfAEJERZV1mLlQcNlmgUkHZihJnEtrGExzNrPxs83B71RSMCdZq\n\tQx/m7elIy13QudF/fFvVHrBM0uI0yDtHXbMDIFX7ntgNihoPV87+zOddA8K82uw3Ph\n\tik/EaPXUFVkIPitPgCiSc88XaXRZ6sCJnULdf5ndwcdCJ3f1uwTFmNbPxvW8DRhN+0\n\tOzNLYr98g/AXMHNgyfdY3UN4d/Q8kgkk99VbDVKqPB3h5W1vBVR92jW30KIGPPmwZI\n\t5Sq9nvTlElXXQ==","Date":"Mon, 17 Feb 2025 14:43:35 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 07/21] apps: common: event_loop: Make it possible\n\tto exit with exception","Message-ID":"","In-Reply-To":"<3umsku7dm5t6gv3wcevawolpemayohansgfsbzcb34cseabcjg@xqakjpm5elb6>","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-8-pobrn@protonmail.com>\n\t<3umsku7dm5t6gv3wcevawolpemayohansgfsbzcb34cseabcjg@xqakjpm5elb6>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"80d8d374a3ed1a1a9277c3cf896fd81ee242d625","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]