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

[Niklas Söderlund]

On 2021-01-19 14:42:27 +0100, Niklas Söderlund wrote:
> 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();
> -       }
>  }

I Think this back as both solutions that modifies dispatchCalls() 
suffers from the same flaw. Events queued before the EventLoop::exit() 
are not executed if they are not already part of the active list of 
calls that are processed.

What do you think of injecting a 'end of call processing' marker into 
the calls_ vector and break the loop in dispatchCalls() if it's 
encounter? Spontaneously I'm thinking of a 'nullptr' marker, but it's 
untested.

> 
> > 
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund