| Message ID | 20240121035948.4226-13-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Laurent Pinchart <laurent.pinchart@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@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(); > }
On Mon, Jan 22, 2024 at 09:55:35PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@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@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(); > > }
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Mon, Jan 22, 2024 at 09:55:35PM +0100, Milan Zamazal wrote: >> Laurent Pinchart <laurent.pinchart@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@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). OTOH such bugs can lead to freezes. Will it then freeze also pipewire or not? If yes then freeze (it doesn't work and nothing happens) may be worse than going down (there is a chance for autorestart). But yes, this is just one of the possible scenarios and trying to keep the service running some way may make sense. I'm not strongly convinced to either side, so I'm OK with it as it is. > I think we should instead propagate errors up the call chain all the way to > the application, OK. > but not in this patch series. Yes, I understand this will be a non-trivial amount of work. So for now: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> > 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(); >> > }
On Tue, Jan 23, 2024 at 12:29:20PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > On Mon, Jan 22, 2024 at 09:55:35PM +0100, Milan Zamazal wrote: > >> Laurent Pinchart <laurent.pinchart@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@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). > > OTOH such bugs can lead to freezes. Will it then freeze also pipewire or not? > If yes then freeze (it doesn't work and nothing happens) may be worse than > going down (there is a chance for autorestart). But yes, this is just one of > the possible scenarios and trying to keep the service running some way may make > sense. I'm not strongly convinced to either side, so I'm OK with it as it is. It may freeze in the sense that it will stop producing images, yes. Calls to libcamera shouldn't block though, and pipewire should be able to cleanly shut down the camera, and possibly restart it. On the other hand, if we crash, we'll take down everything, including audio. > > I think we should instead propagate errors up the call chain all the way to > > the application, > > OK. > > > but not in this patch series. > > Yes, I understand this will be a non-trivial amount of work. > > So for now: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > >> > 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(); > >> > }
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; + 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; +} + /** * \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(); }
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 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@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(-)