[libcamera-devel] [PATCH v2] cam: event_loop: Stop processing calls when event loop is exiting

[Laurent Pinchart]

Ni Niklas,

Thank you for the patch.

On Wed, Jan 20, 2021 at 12:33:15AM +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 evaluating the exit condition between each callback and
> only enter the idle loop if there are no callbacks to process.
> 
> 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>

This seems to be even more of a hack than the previous version :-( Can
we fix this properly please ?

> ---
> * Changes since v1
> - Was [PATCH 0/2] cam: Fix races in event loop and long request
>   processing times
> - Rework dispatchCalls() instead of callLater() for a simpler fix.
> ---
>  src/cam/event_loop.cpp | 22 +++++++++++++---------
>  src/cam/event_loop.h   |  2 +-
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 94c5d1d362455f33..18786b4500be9b6f 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -41,7 +41,9 @@ int EventLoop::exec()
>  	exit_.store(false, std::memory_order_release);
>  
>  	while (!exit_.load(std::memory_order_acquire)) {
> -		dispatchCalls();
> +		if (dispatchCall())
> +			continue;
> +
>  		event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
>  	}
>  
> @@ -70,17 +72,19 @@ void EventLoop::callLater(const std::function<void()> &func)
>  	interrupt();
>  }
>  
> -void EventLoop::dispatchCalls()
> +bool EventLoop::dispatchCall()
>  {
>  	std::unique_lock<std::mutex> locker(lock_);
>  
> -	for (auto iter = calls_.begin(); iter != calls_.end(); ) {
> -		std::function<void()> call = std::move(*iter);
> +	if (calls_.empty())
> +		return false;
>  
> -		iter = calls_.erase(iter);
> +	auto iter = calls_.begin();
> +	std::function<void()> call = std::move(*iter);
> +	calls_.erase(iter);
>  
> -		locker.unlock();
> -		call();
> -		locker.lock();
> -	}
> +	locker.unlock();
> +	call();
> +
> +	return true;
>  }
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index 408073c50594d09d..b84e1c9e432086e3 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -38,7 +38,7 @@ private:
>  	std::mutex lock_;
>  
>  	void interrupt();
> -	void dispatchCalls();
> +	bool dispatchCall();
>  };
>  
>  #endif /* __CAM_EVENT_LOOP_H__ */

-- 
Regards,

Laurent Pinchart