[RFC PATCH v3 07/21] apps: common: event_loop: Make it possible to exit with exception

[Barnabás Pőcze]

Hi


2025. február 4., kedd 14:34 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:

> 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 at 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.

I think it's the cleanest solution. One does not have to always wonder in which
context the code is run, and how an error can be propagated from that context.
And a gtest exception contains much more information than a nondescript integer.

---

`-fno-exceptions` is enabled when compiling for chromeos, so this does not compile
there. I can see two solutions for that:

  * add `-fexceptions` somewhere that includes event_loop.cpp
  * use `#if __cpp_exceptions` and remove the try-catch block accordingly


Regards,
Barnabás Pőcze


> 
> >  }
> >
> > -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
> >
> >