From a25e653f9859831bcb35f6d05bbd325631eb91a4 Mon Sep 17 00:00:00 2001
From: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
Date: Fri, 18 Sep 2015 10:02:57 -0400
Subject: [PATCH] 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 | 74 ++++++++++++++++++++++++++++++++--------------
src/threadloop.h | 28 ++++++++++--------
2 files changed, 67 insertions(+), 35 deletions(-)
diff --git a/src/threadloop.cpp b/src/threadloop.cpp
index 3d1918394c..b1cef7d234 100644
--- a/src/threadloop.cpp
+++ b/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
diff --git a/src/threadloop.h b/src/threadloop.h
index 7ffab94c6c..a15d6d39fe 100644
--- a/src/threadloop.h
+++ b/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__
--
GitLab