[1.0.1][r] fix repeating timers in STD dispatcher

fix: STD dispatcher incorrectly schedules repeating timer event (#8) fix: race condition when starting timers with Qt Dispatcher (used invalid context for lambda function) build: add libc version check for test_memory_footprint test application
igor-krechetov · Jan 14, 2024 · a9a5b00 · a9a5b00
1 parent 0e2e505
commit a9a5b00
Show file tree

Hide file tree

Showing 16 changed files with 330 additions and 72 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,12 @@
 # Changelog
 All notable changes to project will be documented in this file.
 
+## [1.0.1] - 2024-01-11
+### Fixed
+- STD dispatcher incorrectly schedules repeating timer event (https://github.com/igor-krechetov/hsmcpp/pull/8)
+- Race condition when starting timers with Qt Dispatcher (used invalid context for lambda function)
+- Add libc version check for test_memory_footprint test application
+
 ## [1.0.0] - 2023-05-25
 ### Updated
 - add activeStates argument to failedTransition callback

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -3,7 +3,7 @@
 cmake_minimum_required(VERSION 3.16)
 project(hsmcpp)
 
-set(PROJECT_VERSION "1.0.0")
+set(PROJECT_VERSION "1.0.1")
 set(PROJECT_DESCRIPTION "C++ library for hierarchical state machines / finite state machines. Provides a code-free visual approach for defining state machine logic using GUI editors with automatic code and diagram generation. Check out https://hsmcpp.readthedocs.io for detailed documentation.")
 set(CMAKE_VERBOSE_MAKEFILE OFF)
 

diff --git a/README.md b/README.md
@@ -1,5 +1,5 @@
 [![MIT License](https://img.shields.io/badge/license-MIT-blue.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/LICENSE)
-[![Changelog](https://img.shields.io/badge/changelog-v1.0.0-green.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/CHANGELOG.md)
+[![Changelog](https://img.shields.io/badge/changelog-v1.0.1-green.svg)](https://github.com/igor-krechetov/hsmcpp/blob/main/CHANGELOG.md)
 [![Documentation Status](https://readthedocs.org/projects/hsmcpp/badge/?version=latest)](https://hsmcpp.readthedocs.io/en/latest/?badge=latest)
 
 # Releases

diff --git a/include/hsmcpp/HsmEventDispatcherBase.hpp b/include/hsmcpp/HsmEventDispatcherBase.hpp
@@ -218,10 +218,10 @@ class HsmEventDispatcherBase : public IHsmEventDispatcher {
      *
      * @param timerID id of the expired timer
      *
-     * @retval true timer should be restarted
-     * @retval false timer can be deleted (usually because it's a singleshot timer)
+     * @return interval in milliseconds to use for restarting the timer or zero if timer can be deleted
+     *         (usually because it's a singleshot timer)
      */
-    bool handleTimerEvent(const TimerID_t timerID);
+    unsigned int handleTimerEvent(const TimerID_t timerID);
 
     /**
      * @brief Wakeup dispatching thread to process pending events.

diff --git a/scripts/validate.sh b/scripts/validate.sh
@@ -9,8 +9,8 @@ fi
 mkdir ./build
 cd ./build
 
-export Qt6_DIR=~/qt/6.4.2/gcc_64/lib/cmake/Qt6
-# export Qt5_DIR=~/qt/5.13.2/gcc_64/lib/cmake/Qt5
+export Qt6_DIR=~/Qt/6.5.1/gcc_64/lib/cmake/Qt6
+# export Qt5_DIR=~/Qt/5.15.2/gcc_64/lib/cmake/Qt5
 
 if [ ${PWD##*/} = "build" ]
 then
@@ -29,7 +29,7 @@ then
         -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
         -DHSMBUILD_CODECOVERAGE=ON \
         -DHSMBUILD_CLANGTIDY=OFF \
-        -DCMAKE_TOOLCHAIN_FILE=~/qt/6.4.2/gcc_64/lib/cmake/Qt6/qt.toolchain.cmake \
+        -DCMAKE_TOOLCHAIN_FILE=$Qt6_DIR/qt.toolchain.cmake \
         ..
     make -j5
 

diff --git a/src/HsmEventDispatcherBase.cpp b/src/HsmEventDispatcherBase.cpp
@@ -147,34 +147,38 @@ void HsmEventDispatcherBase::startTimer(const HandlerID_t handlerID,
                                         const TimerID_t timerID,
                                         const unsigned int intervalMs,
                                         const bool isSingleShot) {
-    HSM_TRACE_CALL_DEBUG_ARGS("handlerID=%d, timerID=%d, intervalMs=%d, isSingleShot=%d",
+    HSM_TRACE_CALL_DEBUG_ARGS("handlerID=%d, timerID=%d, intervalMs=%u, isSingleShot=%d",
                               handlerID,
                               timerID,
                               intervalMs,
                               BOOL2INT(isSingleShot));
-    LockGuard lck(mHandlersSync);
+    if (intervalMs > 0u) {
+        LockGuard lck(mHandlersSync);
 
-    if (mTimerHandlers.find(handlerID) != mTimerHandlers.end()) {
-        auto it = mActiveTimers.find(timerID);
+        if (mTimerHandlers.find(handlerID) != mTimerHandlers.end()) {
+            auto it = mActiveTimers.find(timerID);
 
-        if (mActiveTimers.end() != it) {
-            it->second.handlerID = handlerID;
-            it->second.intervalMs = intervalMs;
-            it->second.isSingleShot = isSingleShot;
+            if (mActiveTimers.end() != it) {
+                it->second.handlerID = handlerID;
+                it->second.intervalMs = intervalMs;
+                it->second.isSingleShot = isSingleShot;
 
-            // restart timer
-            stopTimerImpl(timerID);
-            startTimerImpl(timerID, it->second.intervalMs, it->second.isSingleShot);
-        } else {
-            TimerInfo newTimer;
+                // restart timer
+                stopTimerImpl(timerID);
+                startTimerImpl(timerID, it->second.intervalMs, it->second.isSingleShot);
+            } else {
+                TimerInfo newTimer;
 
-            newTimer.handlerID = handlerID;
-            newTimer.intervalMs = intervalMs;
-            newTimer.isSingleShot = isSingleShot;
+                newTimer.handlerID = handlerID;
+                newTimer.intervalMs = intervalMs;
+                newTimer.isSingleShot = isSingleShot;
 
-            mActiveTimers[timerID] = newTimer;
-            startTimerImpl(timerID, intervalMs, isSingleShot);
+                mActiveTimers[timerID] = newTimer;
+                startTimerImpl(timerID, intervalMs, isSingleShot);
+            }
         }
+    } else {
+        HSM_TRACE_WARNING("skip. intervalMs must be larger than zero");
     }
 }
 
@@ -247,9 +251,9 @@ void HsmEventDispatcherBase::stopTimerImpl(const TimerID_t timerID) {
     // do nothing. must be implemented in platfrom specific dispatcher
 }
 
-bool HsmEventDispatcherBase::handleTimerEvent(const TimerID_t timerID) {
+unsigned int HsmEventDispatcherBase::handleTimerEvent(const TimerID_t timerID) {
     HSM_TRACE_CALL_DEBUG_ARGS("timerID=%d", SC2INT(timerID));
-    bool restartTimer = false;
+    unsigned int nextIntervalMs = 0;
 
     if (INVALID_HSM_TIMER_ID != timerID) {
         // NOTE: should lock the whole block to prevent situation when timer handler is unregistered during handler execution
@@ -263,10 +267,10 @@ bool HsmEventDispatcherBase::handleTimerEvent(const TimerID_t timerID) {
             if (INVALID_HSM_DISPATCHER_HANDLER_ID != itTimer->second.handlerID) {
                 TimerHandlerFunc_t timerHandler = getTimerHandlerFunc(itTimer->second.handlerID);
 
-                restartTimer = ((true == itTimer->second.isSingleShot) ? false : true);
+                nextIntervalMs = ((true == itTimer->second.isSingleShot) ? 0 : itTimer->second.intervalMs);
 
                 // Remove singleshot timer from active list
-                if (false == restartTimer) {
+                if (0u == nextIntervalMs) {
                     mActiveTimers.erase(itTimer);
                 }
 
@@ -275,7 +279,7 @@ bool HsmEventDispatcherBase::handleTimerEvent(const TimerID_t timerID) {
         }
     }
 
-    return restartTimer;
+    return nextIntervalMs;
 }
 
 void HsmEventDispatcherBase::dispatchEnqueuedEvents() {

diff --git a/src/HsmEventDispatcherGLib.cpp b/src/HsmEventDispatcherGLib.cpp
@@ -186,7 +186,7 @@ gboolean HsmEventDispatcherGLib::onTimerEvent(const TimerData_t* timerData) {
     bool restartTimer = false;
 
     if (nullptr != timerData) {
-        restartTimer = timerData->first->handleTimerEvent(timerData->second);
+        restartTimer = (timerData->first->handleTimerEvent(timerData->second) > 0);
 
         if (false == restartTimer) {
             CriticalSection lckExpired(timerData->first->mRunningTimersSync);

diff --git a/src/HsmEventDispatcherGLibmm.cpp b/src/HsmEventDispatcherGLibmm.cpp
@@ -151,7 +151,7 @@ void HsmEventDispatcherGLibmm::notifyDispatcherAboutEvent() {
 
 bool HsmEventDispatcherGLibmm::onTimerEvent(const TimerID_t timerID) {
     HSM_TRACE_CALL_DEBUG_ARGS("timerID=%d", SC2INT(timerID));
-    const bool restartTimer = handleTimerEvent(timerID);
+    const bool restartTimer = (handleTimerEvent(timerID) > 0);
 
     if (false == restartTimer) {
         CriticalSection cs(mRunningTimersSync);

diff --git a/src/HsmEventDispatcherQt.cpp b/src/HsmEventDispatcherQt.cpp
@@ -91,14 +91,14 @@ void HsmEventDispatcherQt::unregisterAllTimerHandlers() {
 }
 
 void HsmEventDispatcherQt::startTimerImpl(const TimerID_t timerID, const unsigned int intervalMs, const bool isSingleShot) {
-    HSM_TRACE_CALL_DEBUG_ARGS("timerID=%d, intervalMs=%d, isSingleShot=%d",
+    HSM_TRACE_CALL_DEBUG_ARGS("timerID=%d, intervalMs=%u, isSingleShot=%d",
                               SC2INT(timerID),
                               intervalMs,
                               BOOL2INT(isSingleShot));
     auto it = mNativeTimerHandlers.find(timerID);
 
     if (mNativeTimerHandlers.end() == it) {
-        auto funcCreateTimer = [&]() {
+        auto funcCreateTimer = [=]() {
             QTimer* newTimer = new QTimer(this);
 
             newTimer->setProperty("hsmid", QVariant(timerID));
@@ -150,9 +150,9 @@ void HsmEventDispatcherQt::onTimerEvent() {
         const TimerID_t timerID = ptrTimer->property("hsmid").toInt();
 
         HSM_TRACE_CALL_DEBUG_ARGS("timerID=%d", SC2INT(timerID));
-        const bool restartTimer = handleTimerEvent(timerID);
 
-        if (false == restartTimer) {
+        if (0 == handleTimerEvent(timerID)) {
+            // delete timer since it doesn't need to be restarted
             CriticalSection cs(mRunningTimersSync);
             auto itTimer = mNativeTimerHandlers.find(timerID);
 

diff --git a/src/HsmEventDispatcherSTD.cpp b/src/HsmEventDispatcherSTD.cpp
@@ -178,30 +178,43 @@ void HsmEventDispatcherSTD::handleTimers() {
             }
 
             TimerID_t waitingTimerId = itTimeout->first;
-            const int intervalMs = std::chrono::duration_cast<std::chrono::milliseconds>(itTimeout->second.elapseAfter -
+            const int waitDurationMs = std::chrono::duration_cast<std::chrono::milliseconds>(itTimeout->second.elapseAfter -
                                                                                          std::chrono::steady_clock::now())
                                        .count();
 
+            // unlock after itTimeout value is not needed anymore
             mRunningTimersSync.unlock();
+            // NOTE: make sure we don't use itTimeout value after this line
 
-            // NOTE: false-positive. "A function should have a single point of exit at the end" is not vialated because
-            //       "return" statement belogs to a lamda function, not doDispatching.
-            // cppcheck-suppress misra-c2012-15.5
-            const bool waitResult = mTimerEvent.wait_for(lck, intervalMs, [&]() { return mNotifiedTimersThread; });
+            bool waitResult = false;
+
+            // if waitDurationMs <= 0 it means that timer already expired and we only need to trigger event
+            if (waitDurationMs > 0) {
+                // NOTE: false-positive. "A function should have a single point of exit at the end" is not vialated because
+                //       "return" statement belogs to a lamda function, not doDispatching.
+                // cppcheck-suppress misra-c2012-15.5
+                waitResult = mTimerEvent.wait_for(lck, waitDurationMs, [&]() { return mNotifiedTimersThread; });
+            }
 
             mNotifiedTimersThread = false;
 
             if (false == waitResult) {
                 // timeout expired
+
+                // store wakeup time in case we'll need to calculate new elapseAfter value
+                // needed to avoid potential delays caused by CriticalSection and handleTimerEvent()
+                const auto wakeupTime = std::chrono::steady_clock::now();
                 CriticalSection lckExpired(mRunningTimersSync);
 
                 itTimeout = mRunningTimers.find(waitingTimerId);
 
                 if (itTimeout != mRunningTimers.end()) {
-                    if (true == handleTimerEvent(waitingTimerId)) {
+                    const unsigned int nextIntervalMs = handleTimerEvent(waitingTimerId);
+
+                    if (nextIntervalMs > 0u) {
                         // restart timer
-                        itTimeout->second.startedAt = std::chrono::steady_clock::now();
-                        itTimeout->second.elapseAfter = itTimeout->second.startedAt + std::chrono::milliseconds(intervalMs);
+                        itTimeout->second.startedAt = wakeupTime;
+                        itTimeout->second.elapseAfter = itTimeout->second.startedAt + std::chrono::milliseconds(nextIntervalMs);
                     } else {
                         // single shot timer. remove from queue
                         mRunningTimers.erase(itTimeout);

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -27,6 +27,7 @@ set(SRC_UNITTESTS_COMMON ${CMAKE_CURRENT_SOURCE_DIR}/testcases/01_states.cpp
                          ${CMAKE_CURRENT_SOURCE_DIR}/testcases/10_state_actions.cpp
                          ${CMAKE_CURRENT_SOURCE_DIR}/testcases/11_finalstate.cpp
                          ${CMAKE_CURRENT_SOURCE_DIR}/testcases/20_variant.cpp
+                         ${CMAKE_CURRENT_SOURCE_DIR}/testcases/99_regression_tests.cpp
                          ${CMAKE_CURRENT_SOURCE_DIR}/TestsCommon.cpp
 
                          ${CMAKE_CURRENT_SOURCE_DIR}/hsm/TrafficLightHsm.cpp
@@ -74,12 +75,20 @@ if (HSMBUILD_DISPATCHER_STD)
     target_compile_options(${TEST_BIN_STD} PRIVATE ${HSMCPP_STD_CXX_FLAGS})
 
     if (NOT WIN32)
-        # this tool uses Linux specific mallinfo() API
-        add_executable(test_memory_footprint test_memory_footprint.cpp)
-        target_compile_definitions(test_memory_footprint PUBLIC -DTEST_HSM_STD)
-        target_include_directories(test_memory_footprint PRIVATE ${HSMCPP_STD_INCLUDE})
-        target_link_libraries(test_memory_footprint PRIVATE ${HSMCPP_STD_LIB})
-        target_compile_options(test_memory_footprint PRIVATE ${HSMCPP_STD_CXX_FLAGS})
+        # this tool uses Linux specific mallinfo() API and requires glib 2.33+
+        include (CheckSymbolExists)
+
+        CHECK_SYMBOL_EXISTS(mallinfo2 malloc.h MALLINFO2_EXISTS)
+
+        if (MALLINFO2_EXISTS)
+            add_executable(test_memory_footprint test_memory_footprint.cpp)
+            target_compile_definitions(test_memory_footprint PUBLIC -DTEST_HSM_STD)
+            target_include_directories(test_memory_footprint PRIVATE ${HSMCPP_STD_INCLUDE})
+            target_link_libraries(test_memory_footprint PRIVATE ${HSMCPP_STD_LIB})
+            target_compile_options(test_memory_footprint PRIVATE ${HSMCPP_STD_CXX_FLAGS})
+        else()
+            message("[SKIP] test_memory_footprint: mallinfo2 symbol not found (check glib version; version 2.33 or newer is required)")
+        endif()
     endif()
 endif()
 

diff --git a/tests/mainFreeRTOS.cpp b/tests/mainFreeRTOS.cpp
@@ -14,7 +14,7 @@ char** gArgv = nullptr;
 extern "C" {
 void startTests(void* pvParameters) {
     ::testing::InitGoogleMock(&gArgc, gArgv);
-    configureGTest();
+    configureGTest("freertos");
 
     static_cast<void>(RUN_ALL_TESTS());
     vTaskEndScheduler();

diff --git a/tests/test_memory_footprint.cpp b/tests/test_memory_footprint.cpp
@@ -1,4 +1,5 @@
 #include <malloc.h>
+// NOTE: glib 2.33 required
 
 #include <hsmcpp/HsmEventDispatcherSTD.hpp>
 #include <hsmcpp/hsm.hpp>

diff --git a/tests/testcases/06_dispatchers.cpp b/tests/testcases/06_dispatchers.cpp
@@ -120,6 +120,8 @@ TEST(dispatchers, release_sync) {
     EXPECT_LT(*callbackCounter, eventsCount);
 }
 
+#ifndef TEST_HSM_FREERTOS
+
 TEST(dispatchers, stresstest_create_destroy_hsm_later) {
     TEST_DESCRIPTION("check that it's possible to destroy HSM and disconnect from dispatcher when there are pending events");
     // The idea behild this test is to make sure that HSM is not deleted while being inside one of it's callbacks
@@ -219,6 +221,8 @@ TEST(dispatchers, stresstest_create_destroy_hsm_later) {
     EXPECT_EQ(objectsDeleted, iterationsCount);
 }
 
+#endif // TEST_HSM_FREERTOS
+
 TEST(dispatchers, stresstest_create_destroy_dispatcher) {
     TEST_DESCRIPTION("check that it's possible to destroy dispatcher when there are pending events");