[PATCH 12/12] libcamera: object: Add and use thread-bound assertion

[Laurent Pinchart]

On Mon, Jan 22, 2024 at 09:55:35PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 
> > Several functions in libcamera classes are marked as thread-bound,
> > restricting the contexts in which those functions can be called. There
> > is no infrastructure to enfore these restrictions, causing difficult to
>                           ^^^^^^
> enforce
> 
> > debug race conditions when they are not met by callers.
> >
> > As a first step to solve this, add an assertThreadBound() protected
> > function to the Object class to test if the calling thread context is
> > valid, and use it in member functions of Object subclasses marked as
> > thread-bound. This replaces manual tests in a few locations.
> >
> > The thread-bound member functions of classes that do not inherit from
> > Object are not checked, and neither are the functions of classes marked
> > as thread-bound at the class level. These issue should be addressed in
> > the future.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/base/object.h       |  2 ++
> >  src/libcamera/base/event_notifier.cpp |  6 +++++
> >  src/libcamera/base/object.cpp         | 32 ++++++++++++++++++++++++++-
> >  src/libcamera/base/timer.cpp          | 10 +++------
> >  4 files changed, 42 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h
> > index 933336361155..cb7e0a132be2 100644
> > --- a/include/libcamera/base/object.h
> > +++ b/include/libcamera/base/object.h
> > @@ -49,6 +49,8 @@ public:
> >  protected:
> >  	virtual void message(Message *msg);
> >  
> > +	bool assertThreadBound(const char *message);
> > +
> >  private:
> >  	friend class SignalBase;
> >  	friend class Thread;
> > diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp
> > index fd93c0878c6f..a519aec38efb 100644
> > --- a/src/libcamera/base/event_notifier.cpp
> > +++ b/src/libcamera/base/event_notifier.cpp
> > @@ -8,6 +8,7 @@
> >  #include <libcamera/base/event_notifier.h>
> >  
> >  #include <libcamera/base/event_dispatcher.h>
> > +#include <libcamera/base/log.h>
> >  #include <libcamera/base/message.h>
> >  #include <libcamera/base/thread.h>
> >  
> > @@ -20,6 +21,8 @@
> >  
> >  namespace libcamera {
> >  
> > +LOG_DECLARE_CATEGORY(Event)
> > +
> >  /**
> >   * \class EventNotifier
> >   * \brief Notify of activity on a file descriptor
> > @@ -104,6 +107,9 @@ EventNotifier::~EventNotifier()
> >   */
> >  void EventNotifier::setEnabled(bool enable)
> >  {
> > +	if (!assertThreadBound("EventNotifier can't be enabled from another thread"))
> > +		return;
> > +
> 
> Is it OK to continue with libcamera execution when the requested action is not
> fulfilled here?  Can a wrong state of EventNotifier lead to serious trouble and
> damages worse than exiting the program?
> 
> The same question for the other checks below.

In debug builds we certainly want to fail very loudly. In release
builds, however, I don't think exiting is a reasonable option. I expect
libcamera to be run as a service (underneath pipewire) in quite a lot of
use cases, and taking pipewire down would be Bad (TM). I think we should
instead propagate errors up the call chain all the way to the
application, but not in this patch series.

> >  	if (enabled_ == enable) return;
> >  
> > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp
> > index 8af0337f5448..d98a9157773e 100644
> > --- a/src/libcamera/base/object.cpp
> > +++ b/src/libcamera/base/object.cpp
> > @@ -224,6 +224,35 @@ void Object::message(Message *msg)
> >  	}
> >  }
> >  
> > +/**
> > + * \fn Object::assertThreadBound()
> > + * \brief Check if the caller complies with thread-bound constraints
> > + * \param[in] message The message to be printed on error
> > + *
> > + * This function verifies the calling constraints required by the \threadbound
> > + * definition. It shall be called at the beginning of member functions of an
> > + * Object subclass that are explicitly marked as thread-bound in their
> > + * documentation.
> > + *
> > + * If the thread-bound constraints are not met, the function prints \a message
> > + * as an error message. For debug builds, it additionally causes an assertion
> > + * error.
> > + *
> > + * \todo Verify the thread-bound requirements for functions marked as
> > + * thread-bound at the class level.
> > + *
> > + * \return True if the call is thread-bound compliant, false otherwise
> > + */
> > +bool Object::assertThreadBound(const char *message)
> > +{
> > +	if (Thread::current() == thread_)
> > +		return true;
> > +
> > +	LOG(Object, Error) << message;
> > +	ASSERT(false);
> > +	return false;
> 
> Or maybe simply exiting here rather than leaving the decision on the callers?
> 
> (There can be different answers for "in this patch for now" and "in future once
> more tested".)
> 
> > +}
> > +
> >  /**
> >   * \fn R Object::invokeMethod()
> >   * \brief Invoke a method asynchronously on an Object instance
> > @@ -275,7 +304,8 @@ void Object::message(Message *msg)
> >   */
> >  void Object::moveToThread(Thread *thread)
> >  {
> > -	ASSERT(Thread::current() == thread_);
> > +	if (!assertThreadBound("Object can't be moved from another thread"))
> > +		return;
> >  
> >  	if (thread_ == thread)
> >  		return;
> > diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> > index 74b060af32ff..24dbf1e892c3 100644
> > --- a/src/libcamera/base/timer.cpp
> > +++ b/src/libcamera/base/timer.cpp
> > @@ -85,10 +85,8 @@ void Timer::start(std::chrono::milliseconds duration)
> >   */
> >  void Timer::start(std::chrono::steady_clock::time_point deadline)
> >  {
> > -	if (Thread::current() != thread()) {
> > -		LOG(Timer, Error) << "Timer " << this << " << can't be started from another thread";
> > +	if (!assertThreadBound("Timer can't be started from another thread"))
> >  		return;
> > -	}
> >  
> >  	deadline_ = deadline;
> >  
> > @@ -114,13 +112,11 @@ void Timer::start(std::chrono::steady_clock::time_point deadline)
> >   */
> >  void Timer::stop()
> >  {
> > -	if (!isRunning())
> > +	if (!assertThreadBound("Timer can't be stopped from another thread"))
> >  		return;
> >  
> > -	if (Thread::current() != thread()) {
> > -		LOG(Timer, Error) << "Timer " << this << " can't be stopped from another thread";
> > +	if (!isRunning())
> >  		return;
> > -	}
> >  
> >  	unregisterTimer();
> >  }

-- 
Regards,

Laurent Pinchart