| Message ID | 20191006053226.8976-7-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sun, Oct 06, 2019 at 08:32:23AM +0300, Laurent Pinchart wrote: > Starting or stopping a timer from a different thread than the one it > belongs to is inherently racy. Disallow it. > Looks good Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/timer.cpp | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp > index 5d4e52713e6e..8749d66c8662 100644 > --- a/src/libcamera/timer.cpp > +++ b/src/libcamera/timer.cpp > @@ -36,6 +36,11 @@ LOG_DEFINE_CATEGORY(Timer) > * Once started the timer will run until it times out. It can be stopped with > * stop(), and once it times out or is stopped, can be started again with > * start(). > + * > + * Timers run in the thread they belong to, and thus emit the \a ref timeout > + * signal from that thread. To avoid race conditions they must not be started > + * or stopped from a different thread, attempts to do so will be rejected and > + * logged, and may cause undefined behaviour. > */ > > /** > @@ -57,17 +62,24 @@ Timer::~Timer() > * \brief Start or restart the timer with a timeout of \a msec > * \param[in] msec The timer duration in milliseconds > * > - * If the timer is already running it will be stopped and restarted. > + * This method shall be called from the thread the timer is associated with. If > + * the timer is already running it will be stopped and restarted. > */ > > /** > * \brief Start or restart the timer with a timeout of \a duration > * \param[in] duration The timer duration in milliseconds > * > - * If the timer is already running it will be stopped and restarted. > + * This method shall be called from the thread the timer is associated with. If > + * the timer is already running it will be stopped and restarted. > */ > void Timer::start(std::chrono::milliseconds duration) > { > + if (Thread::current() != thread()) { > + LOG(Timer, Error) << "Timer can't be started from another thread"; > + return; > + } > + > deadline_ = utils::clock::now() + duration; > > LOG(Timer, Debug) > @@ -87,13 +99,19 @@ void Timer::start(std::chrono::milliseconds duration) > * After this function returns the timer is guaranteed not to emit the > * \ref timeout signal. > * > - * If the timer is not running this function performs no operation. > + * This method shall be called from the thread the timer is associated with. If > + * the timer is not running this function performs no operation. > */ > void Timer::stop() > { > if (!isRunning()) > return; > > + if (Thread::current() != thread()) { > + LOG(Timer, Error) << "Timer can't be stopped from another thread"; > + return; > + } > + > unregisterTimer(); > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp index 5d4e52713e6e..8749d66c8662 100644 --- a/src/libcamera/timer.cpp +++ b/src/libcamera/timer.cpp @@ -36,6 +36,11 @@ LOG_DEFINE_CATEGORY(Timer) * Once started the timer will run until it times out. It can be stopped with * stop(), and once it times out or is stopped, can be started again with * start(). + * + * Timers run in the thread they belong to, and thus emit the \a ref timeout + * signal from that thread. To avoid race conditions they must not be started + * or stopped from a different thread, attempts to do so will be rejected and + * logged, and may cause undefined behaviour. */ /** @@ -57,17 +62,24 @@ Timer::~Timer() * \brief Start or restart the timer with a timeout of \a msec * \param[in] msec The timer duration in milliseconds * - * If the timer is already running it will be stopped and restarted. + * This method shall be called from the thread the timer is associated with. If + * the timer is already running it will be stopped and restarted. */ /** * \brief Start or restart the timer with a timeout of \a duration * \param[in] duration The timer duration in milliseconds * - * If the timer is already running it will be stopped and restarted. + * This method shall be called from the thread the timer is associated with. If + * the timer is already running it will be stopped and restarted. */ void Timer::start(std::chrono::milliseconds duration) { + if (Thread::current() != thread()) { + LOG(Timer, Error) << "Timer can't be started from another thread"; + return; + } + deadline_ = utils::clock::now() + duration; LOG(Timer, Debug) @@ -87,13 +99,19 @@ void Timer::start(std::chrono::milliseconds duration) * After this function returns the timer is guaranteed not to emit the * \ref timeout signal. * - * If the timer is not running this function performs no operation. + * This method shall be called from the thread the timer is associated with. If + * the timer is not running this function performs no operation. */ void Timer::stop() { if (!isRunning()) return; + if (Thread::current() != thread()) { + LOG(Timer, Error) << "Timer can't be stopped from another thread"; + return; + } + unregisterTimer(); }
Starting or stopping a timer from a different thread than the one it belongs to is inherently racy. Disallow it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/timer.cpp | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)