# Is this thread pool implementation OK?

There are things that I am not 100% sure. Is it possible that dangling references can be create in add_task method? Is it possible that this implementation will deadlock? I had to use shared_ptr in order to store packaged_task in the queue. Is there a better way to do it?

class ThreadPool {
public:
for (int i=0; i<number_of_workers; ++i) {
}
}

template<typename Func, typename... Args>
using ReturnType = decltype(func(args...));
std::bind(std::forward<Func>(func), std::forward<Args>(args)...));
{
std::unique_lock lk{pool_mutex};
}
cond.notify_one();
return fut;
}

void stop() {
{
std::unique_lock lk{pool_mutex};
stopped = true;
}
cond.notify_all();
for (auto& th : worker_threads) {
if (th.joinable()) {
th.join();
}
}
}

private:
while(true) {
std::unique_lock lk{pool_mutex};
break;
}

cond.wait(lk, [this]() { return !task_queue.empty() || stopped; });
}

lk.unlock();
callable();
}
else if (stopped) {
break;
}
}
}

private:
std::mutex pool_mutex;
bool stopped = false;
std::condition_variable cond;
};


Note, that add_task returns a future that can be used to wait for a specific task to complete.

~ThreadPool() = default;


We need to call stop() in the destructor!

template<typename Func, typename... Args>
using ReturnType = decltype(func(args...));


For perfect forwarding we need to use && for args:

template<typename Func, typename... Args>
using ReturnType = decltype(func(std::forward<Args>(args)...));


ThreadPool(int number_of_workers) {


We need to check that this number is greater than zero. Also it might be better to use an unsigned int to match the result of std::thread::hardware_concurrency().

void thread_func() {
while(true) {
std::unique_lock lk{pool_mutex};
break;
}
...


I'm not sure that we should wait for the task_queue to be empty before stopping. I'd suggest that stop() should prevent threads taking any more tasks from the queue (i.e. provide a way to "cancel" tasks that haven't started executing yet).

If we wait for the queue to be empty when calling stop(), we're waiting for all the tasks to complete, so calling stop() doesn't really do much. (The calling code is already in control of whether or not it adds more tasks and waits for futures).

        if (task_queue.empty()) {
cond.wait(lk, [this]() { return !task_queue.empty() || stopped; });
}


We don't really need to put cond.wait() in an if statement. It will check the condition before waiting.

I'd probably go for something more like:

void thread_func() {
while(true) {

std::unique_lock lk{pool_mutex};
cond.wait(lk, [this]() { return !task_queue.empty() || stopped; });

if (stopped)
break;