PeriodicThread fix starting after stop, more robust stop, atomic runn…

…ing check
HypersonicED · Jul 14, 2023 · 1a364d5 · 1a364d5
1 parent afefe6d
commit 1a364d5
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 13 deletions.
diff --git a/include/aniray/PeriodicThread.hpp b/include/aniray/PeriodicThread.hpp
@@ -27,6 +27,7 @@
 #ifndef ANIRAY_PERIODICTHREAD_HPP
 #define ANIRAY_PERIODICTHREAD_HPP
 
+#include <atomic>
 #include <chrono>
 #include <memory>
 #include <shared_mutex>
@@ -60,7 +61,7 @@ class PeriodicThread { // NOLINT(cppcoreguidelines-special-member-functions,hicp
         virtual void periodicAction() = 0;
     private:
         bool mWasRunning = false;
-        bool mRunning = false;
+        std::atomic_bool mRunning = false;
         std::chrono::milliseconds mUpdateRateMs;
         boost::asio::io_context mIOContext;
         std::unique_ptr<boost::asio::steady_timer> mTimer;

diff --git a/src/PeriodicThread.cpp b/src/PeriodicThread.cpp
@@ -24,16 +24,20 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+#include <atomic>
 #include <chrono>
 #include <memory>
 #include <mutex>
 #include <shared_mutex>
+#include <system_error>
 
 #include <boost/asio/executor_work_guard.hpp>
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/steady_timer.hpp>
 #include <boost/system/error_code.hpp> // IWYU pragma: keep
 #include <boost/thread.hpp> // IWYU pragma: keep
+#include <boost/thread/exceptions.hpp>
+#include <boost/thread/interruption.hpp>
 // IWYU pragma: no_include <boost/asio/basic_waitable_timer.hpp>
 // IWYU pragma: no_include <boost/thread/thread_only.hpp>
 // IWYU pragma: no_forward_declare boost::system::error_code
@@ -53,16 +57,22 @@ PeriodicThread::PeriodicThread(std::chrono::milliseconds updateRateMs)
 
 void PeriodicThread::start() {
     if (mWasRunning) {
+        stop();
         mIOContext.restart();
-    } else {
-        mIOThread = std::make_unique<boost::thread>([ObjectPtr = &mIOContext] { ObjectPtr->run(); });
     }
+    mIOThread = std::make_unique<boost::thread>([ObjectPtr = &mIOContext] { ObjectPtr->run(); });
     mWasRunning = true;
     mRunning = true;
 }
 
 void PeriodicThread::stop() {
     mIOContext.stop();
+    if (mIOThread) {
+        mIOThread->interrupt();
+        if (mIOThread->joinable()) {
+            mIOThread->join();
+        }
+    }
     mRunning = false;
 }
 
@@ -81,19 +91,33 @@ void PeriodicThread::updateRate(std::chrono::milliseconds updateRateMs) {
 }
 
 PeriodicThread::~PeriodicThread() {
-   const std::unique_lock<std::shared_mutex> lock(mHandlerMutex);
-   stop();
-    // if (mIOThread->joinable()) {
-    //     mIOThread->join();
-    // }
+    const std::unique_lock<std::shared_mutex> lock(mHandlerMutex);
+    try {
+        stop();
+    } catch (const boost::thread_interrupted&) {
+        // Suppress for destructor
+        return;
+    } catch (const std::system_error& e) {
+        // Suppress for destructor
+        return;
+    } catch (...) {
+        // Suppress for destructor
+        return;
+    }
+    // See https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL57-CPP.+Do+not+let+exceptions+escape+from+destructors+or+deallocation+functions
+    // for returning in catch()
 }
 
 void PeriodicThread::timerHandler () {
-    const std::shared_lock<std::shared_mutex> lockHandler(mHandlerMutex);
-    periodicAction();
-    const std::shared_lock<std::shared_mutex> lockUpdateRate(mUpdateRateMutex);
-    mTimer->expires_at(mTimer->expiry() + mUpdateRateMs);
-    mTimer->async_wait([this] (const boost::system::error_code&) { timerHandler(); });
+    boost::this_thread::interruption_point();
+    {
+        const std::shared_lock<std::shared_mutex> lockHandler(mHandlerMutex);
+        periodicAction();
+        const std::shared_lock<std::shared_mutex> lockUpdateRate(mUpdateRateMutex);
+        mTimer->expires_at(mTimer->expiry() + mUpdateRateMs);
+        mTimer->async_wait([this] (const boost::system::error_code&) { timerHandler(); });
+    }
+    boost::this_thread::interruption_point();
 }
 
 } // namespace aniray