I am trying to implement a singleton worker thread which has multiple functions which can be requested by the user. I have seen implementations for worker threads using always the same function but I need different calls. Making a worker thread for each function is incovenient because the functions will share some resources. What I did to make it work is add a queue of function calls where I simply push the function calls onto the queue and trigger the condition variable. The question now becomes whether this implementation is fine because I feel like there is a smarter way to do this.
#include <functional>
#include <queue>
#include <condition_variable>
#include <mutex>
#include <thread>
class Worker_Thread
{
private:
Worker_Thread() : th_{}, mtx_{}, q_{}
{
th_ = std::thread(&Worker_Thread::run, this);
}
std::thread th_;
std::mutex mtx_;
std::condition_variable cv_;
std::queue<std::function<void()>> q_;
void run();
void func_a(int val) {};
void func_b(float val) {};
public:
Worker_Thread(Worker_Thread const&) = delete;
void operator=(Worker_Thread const&) = delete;
static Worker_Thread& getInstance()
{
static Worker_Thread instance;
return instance;
}
void do_a(int val);
void do_b(float val);
};
void Worker_Thread::run()
{
while (true) {
std::unique_lock<std::mutex> lock(mtx_);
cv_.wait(lock);
std::function<void()> fnc = std::move(q_.front());
fnc();
q_.pop();
}
}
void Worker_Thread::do_a(int val)
{
{
// Push a function onto the queue
std::lock_guard<std::mutex> lock(mtx_);
std::function<void()> fnc = std::bind(&Worker_Thread::func_a, this, val);
q_.push(fnc);
}
// Notify the run thread to execute the queued function
cv_.notify_one();
}
void Worker_Thread::do_b(float val)
{
{
// Push a function onto the queue
std::lock_guard<std::mutex> lock(mtx_);
std::function<void()> fnc = std::bind(&Worker_Thread::func_b, this, val);
q_.push(fnc);
}
// Notify the run thread to execute the queued function
cv_.notify_one();
}
int main()
{
Worker_Thread& wth = Worker_Thread::getInstance();
wth.do_a(1);
wth.do_b(2.3);
return 0;
}
This is more of a code review, which, incidentally, got its own site. However, a few notes:
run function doesn't release the lock while running the function. That will prevent other threads from queueing new work. Your use of a condition variable is also wrong. You have to check the associated condition before waiting. It should look something like this:void Worker_Thread::run()
{
while (true) {
std::function<void()> fnc;
{
std::unique_lock<std::mutex> lock(mtx_);
while(q_.empty())
cv_.wait(lock);
fnc = std::move(q_.front());
q_.pop();
}
fnc();
}
}
You have no way of terminating the thread or ensuring that the queue is processed before the main thread exits. But this would be easy to add in your design by queueing a function that terminates the run method. Then you can join the thread. You may also want to make sure to prevent other threads from queuing work after the close command has been given.
Your do_a and do_b methods should construct the function object before locking the mutex. Constructing a function is expensive since it requires dynamic memory allocation. You should always do as little work as possible while holding a mutex. You should also move it into the queue.
void Worker_Thread::do_a(int val)
{
std::function<void()> fnc = std::bind(&Worker_Thread::func_a, this, val);
{
// Push a function onto the queue
std::lock_guard<std::mutex> lock(mtx_);
q_.push(std::move(fnc));
}
// Notify the run thread to execute the queued function
cv_.notify_one();
}
Consider switching from function to packaged_task so that callers can wait on the result. This would also allow you to catch exceptions and return them to the original caller rather than terminating the thread and crashing the program, as it would currently do
Lambdas are cheaper, easier to use and read than bind with method pointers. Consider this pattern instead:
void Worker_Thread::do_a(int val)
{
std::function<void()> fnc {[=]() -> void {
this->func_a(val);
}};
...
}
That way you also don't need a separate func_a and can just inline the code.