[libcamera-devel] [PATCH 1/2] cam: event_loop: Stop queuing calls when the event loop are exiting

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Jan 19, 2021 at 01:31:09PM +0100, Niklas Söderlund wrote:
> Terminating the event loop with EventLoop::exit() does not grantee that
> it will terminate. If the event loops 'call later' queue can be kept
> non-empty by continuously queuing new calls using EventLoop::callLater()
> either from a different thread or from callbacks in the loop itself
> EventLoop::dispatchCalls() will never complete and the loop will run
> forever.
> 
> Solve this by not accepting new callbacks once the event loop is
> exiting.
> 
> Reported-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support")
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/event_loop.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 94c5d1d362455f33..34a55da30c537ac7 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -62,7 +62,7 @@ void EventLoop::interrupt()
>  
>  void EventLoop::callLater(const std::function<void()> &func)
>  {
> -	{
> +	if (!exit_.load(std::memory_order_acquire)) {
>  		std::unique_lock<std::mutex> locker(lock_);
>  		calls_.push_back(func);
>  	}

That's a bit of a layering violation. Would it make sense to fix this
differently, by ensuring that EventLoop::dispatchCalls() will only
handle deferred calls that are on the list when the function is called ?
This would also ensure that callLater(), when called from the same
thread, will guarantee that pending events get processed before the
delayed call is processed.

I mean something along those lines (only compile-tested).

diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
index 94c5d1d36245..320ade525a3f 100644
--- a/src/cam/event_loop.cpp
+++ b/src/cam/event_loop.cpp
@@ -74,7 +74,7 @@ void EventLoop::dispatchCalls()
 {
 	std::unique_lock<std::mutex> locker(lock_);

-	for (auto iter = calls_.begin(); iter != calls_.end(); ) {
+	for (auto iter = calls_.begin(), end = calls_.end(); iter != end; ) {
 		std::function<void()> call = std::move(*iter);

 		iter = calls_.erase(iter);


-- 
Regards,

Laurent Pinchart