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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-01-19 14:43:17 +0200, Laurent Pinchart wrote:
> 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);

I tried something like this but thought it was nastier then I proposed 
in this patch. I do however agree that what is proposed in my patch is 
also not the most cleanest of solutions. The change you propose here 
does not work as intended, it is still venerable to an ever increasing 
call_ vector.

If we would like to go with this solution how about something like this?  
It would also reduce the lock_ contention as a side effect.

diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
index 94c5d1d362455f33..4bfcc503ac1f3b83 100644
--- a/src/cam/event_loop.cpp
+++ b/src/cam/event_loop.cpp
@@ -72,15 +72,13 @@ void EventLoop::callLater(const std::function<void()> &func)
 
 void EventLoop::dispatchCalls()
 {
-       std::unique_lock<std::mutex> locker(lock_);
+       std::list<std::function<void()>> calls;
 
-       for (auto iter = calls_.begin(); iter != calls_.end(); ) {
-               std::function<void()> call = std::move(*iter);
+       {
+               std::unique_lock<std::mutex> locker(lock_);
+               calls = std::move(calls_);
+       }
 
-               iter = calls_.erase(iter);
-
-               locker.unlock();
+       for (std::function<void()> call : calls)
                call();
-               locker.lock();
-       }
 }

> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund