threadloop: fix deadlock issue

start() can deadlock into join if a previous stop has been called and
the thread hasn't seen it before started again.

This patch solves that by using a transactional stop (using FSM).
This also adds MoveConstructible and MoveAssignement behaviour.

Issue: #80556
Change-Id: Ie04e3a20007e69b4d372b9619d67710f4e2af1b7

src/threadloop.cpp

@@ -34,27 +34,43 @@
 
 namespace ring {
 
-void ThreadLoop::mainloop()
+void
+ThreadLoop::mainloop(const std::function<bool()> setup,
+                     const std::function<void()> process,
+                     const std::function<void()> cleanup)
 {
     try {
-        if (setup_()) {
-            while (running_)
-                process_();
-            cleanup_();
+        if (setup()) {
+            while (state_ == RUNNING)
+                process();
+            cleanup();
         } else {
             RING_ERR("setup failed");
         }
-    } catch (const ThreadLoopException &e) {
+    } catch (const ThreadLoopException& e) {
         RING_ERR("%s", e.what());
     }
 }
 
-ThreadLoop::ThreadLoop(const std::function<bool()> &setup,
-                       const std::function<void()> &process,
-                       const std::function<void()> &cleanup)
-    : setup_(setup), process_(process), cleanup_(cleanup)
+ThreadLoop::ThreadLoop(const std::function<bool()>& setup,
+                       const std::function<void()>& process,
+                       const std::function<void()>& cleanup)
+    : setup_(setup)
+    , process_(process)
+    , cleanup_(cleanup)
+    , thread_()
 {}
 
+ThreadLoop::ThreadLoop(ThreadLoop&& other)
+    : setup_(std::move(other.setup_))
+    , process_(std::move(other.process_))
+    , cleanup_(std::move(other.cleanup_))
+    , state_(other.state_.load())
+    , thread_(std::move(other.thread_))
+{
+    other.state_ = READY;
+}
+
 ThreadLoop::~ThreadLoop()
 {
     if (isRunning()) {
@@ -63,24 +79,35 @@ ThreadLoop::~ThreadLoop()
     }
 }
 
-void ThreadLoop::start()
+void
+ThreadLoop::start()
 {
-    if (!running_.exchange(true)) {
-        // a previous stop() call may be pending
-        if (thread_.joinable())
-            thread_.join();
-        thread_ = std::thread(&ThreadLoop::mainloop, this);
-    } else {
-        RING_ERR("Thread already started");
+    const auto s = state_.load();
+
+    if (s == RUNNING) {
+        RING_ERR("already started");
+        return;
     }
+
+    // stop pending but not processed by thread yet?
+    if (s == STOPPING and thread_.joinable()) {
+        RING_DBG("stop pending");
+        thread_.join();
+    }
+
+    state_ = RUNNING;
+    thread_ = std::thread(&ThreadLoop::mainloop, this, setup_, process_, cleanup_);
 }
 
-void ThreadLoop::stop()
+void
+ThreadLoop::stop()
 {
-    running_ = false;
+    if (state_ == RUNNING)
+        state_ = STOPPING;
 }
 
-void ThreadLoop::join()
+void
+ThreadLoop::join()
 {
     stop();
     if (thread_.joinable())
@@ -93,9 +120,10 @@ void ThreadLoop::exit()
     throw ThreadLoopException();
 }
 
-bool ThreadLoop::isRunning() const
+bool
+ThreadLoop::isRunning() const noexcept
 {
-    return running_;
+    return thread_.joinable() and state_ == RUNNING;
 }
 
 } // namespace ring

src/threadloop.h

@@ -29,8 +29,7 @@
  *  as that of the covered work.
  */
 
-#ifndef __THREADLOOP_H__
-#define __THREADLOOP_H__
+#pragma once
 
 #include <atomic>
 #include <thread>
@@ -50,17 +49,22 @@ struct ThreadLoopException : public std::runtime_error {
 
 class ThreadLoop {
 public:
-    ThreadLoop(const std::function<bool()> &setup,
-               const std::function<void()> &process,
-               const std::function<void()> &cleanup);
+    enum ThreadState {READY, RUNNING, STOPPING};
+
+    ThreadLoop(const std::function<bool()>& setup,
+               const std::function<void()>& process,
+               const std::function<void()>& cleanup);
+
+    ThreadLoop(ThreadLoop&&);
+
     ~ThreadLoop();
 
     void start();
-
     void exit();
     void stop();
     void join();
-    bool isRunning() const;
+
+    bool isRunning() const noexcept;
 
 private:
     // These must be provided by users of ThreadLoop
@@ -68,12 +72,12 @@ private:
     std::function<void()> process_;
     std::function<void()> cleanup_;
 
-    void mainloop();
+    void mainloop(const std::function<bool()> setup,
+                  const std::function<void()> process,
+                  const std::function<void()> cleanup);
 
-    std::atomic<bool> running_ = {false};
-    std::thread thread_ = {};
+    std::atomic<ThreadState> state_ {READY};
+    std::thread thread_;
 };
 
 } // namespace ring
-
-#endif // __THREADLOOP_H__