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

[Milan Zamazal]

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.

> 
>  	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();
>  }