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

[Jacopo Mondi]

Hi Niklas,

On Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:
> Terminating the event loop with EventLoop::exit() does not grantee that

s/grantee/guarantee

> it will terminate. If the event loops 'call later' queue can be kept

event loop

> 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 only executing the already queued calls each invocation of
> dispatchCalls() and only enter the idle loop if dispatchCalls() had no
> calls to dispatch.
>
> 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 | 21 +++++++++------------
>  src/cam/event_loop.h   |  2 +-
>  2 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 94c5d1d362455f33..f0b1ecbb6244c40a 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -41,8 +41,8 @@ int EventLoop::exec()
>  	exit_.store(false, std::memory_order_release);
>
>  	while (!exit_.load(std::memory_order_acquire)) {
> -		dispatchCalls();
> -		event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
> +		if (!dispatchCalls())
> +			event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
>  	}
>
>  	return exitCode_;
> @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function<void()> &func)
>  	interrupt();
>  }
>
> -void EventLoop::dispatchCalls()
> +bool EventLoop::dispatchCalls()
>  {
> -	std::unique_lock<std::mutex> locker(lock_);
> +	lock_.lock();
> +	std::list<std::function<void()>> calls = std::move(calls_);
> +	lock_.unlock();

After this last unlock:
calls contains all the callbacks
calls_ now empty
calls_ can be populated with more callbacks as there's no lock held

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

> -		locker.lock();
> -	}
> +
> +	return !calls.empty();

How can calls be empty as you never remove items ?
As callbacks can be add to calls_ while you are iteraring here, won't
them be lost forever ?

I feel like I'm missing something

>  }
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index 408073c50594d09d..b2535f7bdd96742a 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 dispatchCalls();
>  };
>
>  #endif /* __CAM_EVENT_LOOP_H__ */
> --
> 2.30.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel