[RFC,v3,07/21] apps: common: event_loop: Make it possible to exit with exception
diff mbox series

Message ID 20250130115001.1129305-8-pobrn@protonmail.com
State New
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 11:50 a.m. UTC
Instead of taking a single integer in `EventLoop:exit()`, change it
so that an `std::exception_ptr` can also be passed. And in `EventLoop::exec()`
rethrow the stored exception (if any).

Furthermore, catch exceptions from deferred calls, and stop the event
loop with the caught exception.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/apps/common/event_loop.cpp | 35 +++++++++++++++++++++++++++++-----
 src/apps/common/event_loop.h   |  6 ++++--
 2 files changed, 34 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi Feb. 4, 2025, 1:34 p.m. UTC | #1
Hi Barnabás

On Thu, Jan 30, 2025 at 11:50:36AM +0000, Barnabás Pőcze wrote:
> Instead of taking a single integer in `EventLoop:exit()`, change it
> so that an `std::exception_ptr` can also be passed. And in `EventLoop::exec()`
> rethrow the stored exception (if any).
>
> Furthermore, catch exceptions from deferred calls, and stop the event
> loop with the caught exception.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/apps/common/event_loop.cpp | 35 +++++++++++++++++++++++++++++-----
>  src/apps/common/event_loop.h   |  6 ++++--
>  2 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
> index 0d7a4a024..13c2d6057 100644
> --- a/src/apps/common/event_loop.cpp
> +++ b/src/apps/common/event_loop.cpp
> @@ -12,6 +12,7 @@
>  #include <event2/thread.h>
>  #include <algorithm>
>  #include <iostream>
> +#include <utility>
>
>  EventLoop *EventLoop::instance_ = nullptr;
>
> @@ -38,7 +39,14 @@ EventLoop::EventLoop()
>  				self->calls_.pop_front();
>  			}
>
> -			call();
> +			try {
> +				call();
> +			}
> +			catch (...) {

we don't have a preferred syntax for try {} catch () {}
but I feel like it should probably be

                        try {

                        } catch () {

                        }

to match how we handle if() branches ?

> +				::event_active(self->callsTrigger_, 0, 0);
> +				self->exit(std::current_exception());
> +				break;
> +			}
>  		}
>  	}, this);
>  	assert(callsTrigger_);
> @@ -63,14 +71,31 @@ EventLoop *EventLoop::instance()
>
>  int EventLoop::exec()
>  {
> -	exitCode_ = -1;
> +	{
> +		std::lock_guard locker(lock_);
> +		result_ = 0;
> +	}
> +
>  	event_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);
> -	return exitCode_;
> +
> +	auto result = [&] {
> +		std::lock_guard locker(lock_);
> +		return std::exchange(result_, 0);
> +	}();
> +
> +	if (auto *exc = std::get_if<std::exception_ptr>(&result))
> +		std::rethrow_exception(std::move(*exc));
> +
> +	return std::get<int>(result);

All of this is morbidly fascinating as only C++ can be.

I understand this whole patch is only needed to support the following
hunk in 19/21

-       if (queueRequest(request))
-               loop_->exit(-EINVAL);
+	queueRequest(request);

iow to remove the need for explicit loop interruptions but to
propagate the exceptions thrown by gtest ASSERT_EQ() and friends in
the even loop and exit there.

All of this is neat but I wonder if the complexity of this patch is
necessary just to support that single use case. Maybe it's complex for
me but not for others, I'll ask their opinion.

>  }
>
> -void EventLoop::exit(int code)
> +void EventLoop::exit(std::variant<int, std::exception_ptr> result)
>  {
> -	exitCode_ = code;
> +	{
> +		std::lock_guard locker(lock_);
> +		result_ = std::move(result);
> +	}
> +
>  	event_base_loopbreak(base_);
>  }
>
> diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
> index 6d7d0497a..380c3483a 100644
> --- a/src/apps/common/event_loop.h
> +++ b/src/apps/common/event_loop.h
> @@ -9,11 +9,13 @@
>
>  #include <chrono>
>  #include <deque>
> +#include <exception>
>  #include <functional>
>  #include <list>
>  #include <memory>
>  #include <mutex>
>  #include <optional>
> +#include <variant>
>
>  #include <libcamera/base/class.h>
>
> @@ -35,7 +37,7 @@ public:
>  	static EventLoop *instance();
>
>  	int exec();
> -	void exit(int code = 0);
> +	void exit(std::variant<int, std::exception_ptr> result = 0);
>
>  	void callLater(std::function<void()> &&func, std::optional<std::uintptr_t> cookie = {});
>  	void cancelLater(std::optional<std::uintptr_t> cookie = {});
> @@ -63,7 +65,7 @@ private:
>  	static EventLoop *instance_;
>
>  	struct event_base *base_;
> -	int exitCode_;
> +	std::variant<int, std::exception_ptr> result_ = 0;
>
>  	std::deque<std::pair<std::function<void()>, std::optional<std::uintptr_t>>> calls_;
>  	::event *callsTrigger_ = nullptr;
> --
> 2.48.1
>
>

Patch
diff mbox series

diff --git a/src/apps/common/event_loop.cpp b/src/apps/common/event_loop.cpp
index 0d7a4a024..13c2d6057 100644
--- a/src/apps/common/event_loop.cpp
+++ b/src/apps/common/event_loop.cpp
@@ -12,6 +12,7 @@ 
 #include <event2/thread.h>
 #include <algorithm>
 #include <iostream>
+#include <utility>
 
 EventLoop *EventLoop::instance_ = nullptr;
 
@@ -38,7 +39,14 @@  EventLoop::EventLoop()
 				self->calls_.pop_front();
 			}
 
-			call();
+			try {
+				call();
+			}
+			catch (...) {
+				::event_active(self->callsTrigger_, 0, 0);
+				self->exit(std::current_exception());
+				break;
+			}
 		}
 	}, this);
 	assert(callsTrigger_);
@@ -63,14 +71,31 @@  EventLoop *EventLoop::instance()
 
 int EventLoop::exec()
 {
-	exitCode_ = -1;
+	{
+		std::lock_guard locker(lock_);
+		result_ = 0;
+	}
+
 	event_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);
-	return exitCode_;
+
+	auto result = [&] {
+		std::lock_guard locker(lock_);
+		return std::exchange(result_, 0);
+	}();
+
+	if (auto *exc = std::get_if<std::exception_ptr>(&result))
+		std::rethrow_exception(std::move(*exc));
+
+	return std::get<int>(result);
 }
 
-void EventLoop::exit(int code)
+void EventLoop::exit(std::variant<int, std::exception_ptr> result)
 {
-	exitCode_ = code;
+	{
+		std::lock_guard locker(lock_);
+		result_ = std::move(result);
+	}
+
 	event_base_loopbreak(base_);
 }
 
diff --git a/src/apps/common/event_loop.h b/src/apps/common/event_loop.h
index 6d7d0497a..380c3483a 100644
--- a/src/apps/common/event_loop.h
+++ b/src/apps/common/event_loop.h
@@ -9,11 +9,13 @@ 
 
 #include <chrono>
 #include <deque>
+#include <exception>
 #include <functional>
 #include <list>
 #include <memory>
 #include <mutex>
 #include <optional>
+#include <variant>
 
 #include <libcamera/base/class.h>
 
@@ -35,7 +37,7 @@  public:
 	static EventLoop *instance();
 
 	int exec();
-	void exit(int code = 0);
+	void exit(std::variant<int, std::exception_ptr> result = 0);
 
 	void callLater(std::function<void()> &&func, std::optional<std::uintptr_t> cookie = {});
 	void cancelLater(std::optional<std::uintptr_t> cookie = {});
@@ -63,7 +65,7 @@  private:
 	static EventLoop *instance_;
 
 	struct event_base *base_;
-	int exitCode_;
+	std::variant<int, std::exception_ptr> result_ = 0;
 
 	std::deque<std::pair<std::function<void()>, std::optional<std::uintptr_t>>> calls_;
 	::event *callsTrigger_ = nullptr;