Threadpool implementation in C++11

Question

I am implementing a very primitive threadpool with C++11 using mutex and conditional variable. The class ThreadPool is the main thread holder and will spawn a number of threads upon construction, and join all threads on destruction. The main thread adds task of type function<void()> into the queue and then wakes up any waiting threads through condition variable.

As a new C++11 learner, is there anything I can improve for the following code (or fix bugs)? For example, making the task function more generic and implementing the lock-free mechanism (best practice?).

#include <vector>
#include <queue>
#include <memory>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <future>
#include <functional>
#include <iostream>
#include <stdexcept>
#include <unistd.h>
// Class: ThreadPool
class ThreadPool {
 // Class: Worker
 class Worker {
 public:
 // Constructor.
 explicit Worker(ThreadPool* master) : _master{master} {};
 // Functor.
 void operator ()() {
 ::std::function<void()> task;
 while(true) {
 { // Acquire lock.
 ::std::unique_lock<::std::mutex> lock(_master->_mutex);
 _master->_condition.wait(
 lock,
 [&] () { return !_master->_tasks.empty() || _master->_terminate.load(); }
 ); 
 if(_master->_terminate.load()) return;
 task = _master->_tasks.front();
 _master->_tasks.pop_front();
 } // Release lock.
 task();
 }
 }
 private:
 ThreadPool* _master;
 };
 public:
 ThreadPool(size_t);
 template<class F>
 void enqueue(F f);
 ~ThreadPool();
 ThreadPool(const ThreadPool&) = delete;
 ThreadPool& operator = (const ThreadPool&) = delete;
 private:
 friend class Worker;
 ::std::vector< ::std::thread > _workers;
 ::std::deque< ::std::function<void()> > _tasks;
 ::std::mutex _mutex;
 ::std::condition_variable _condition;
 ::std::atomic_bool _terminate {false};
};
// Constructor
ThreadPool::ThreadPool(size_t threads) {
 for(size_t i = 0;i<threads;++i) {
 _workers.push_back(::std::thread(Worker(this)));
 }
}
// Destructor.
ThreadPool::~ThreadPool() {
 _terminate.store(true);
 _condition.notify_all();
 for(auto& t : _workers) t.join();
}
// Procedure: enqueue
template<class F>
void ThreadPool::enqueue(F f) {
 { // Acquire lock
 ::std::unique_lock<::std::mutex> guard(_mutex);
 _tasks.push_back(::std::function<void()>(f));
 } // Release lock
 // wake up one thread
 _condition.notify_one();
}
void foo() {
 printf("hello by %p\n", ::std::this_thread::get_id());
 sleep(1); 
}
int main() {
 ThreadPool tp(4);
 while(1) {
 printf("main %p: enque\n", ::std::this_thread::get_id());
 tp.enqueue(foo);
 sleep(1);
 }
 return 0;
}

• k_kaz: When you do task = _master->_tasks.front(); _master->_tasks.pop_front(); } task(); don't you delete the function object()? This runs well but if you were to invoke a big function with arguments eventually the object will be deleted. Am i wrong?

Answer 1

I don't see why this should have a hypothetical name:

void foo() {
 printf("hello by %p\n", ::std::this_thread::get_id());
 sleep(1); 
}

If this is meant as a test function, then name it as such. As obvious as the functionality may be, there's still need for such names in an otherwise serious program.

Also, you can use std::this_thread::sleep_for, which is as of C++11:

std::this_thread::sleep_for(std::chrono::seconds(1));

If you also have C++14, use a literal instead:

std::this_thread::sleep_for(1s);

(You'll also need <chrono> for all of this.)

Answer 2

One small thing because I don't do much C++: Most of your comments are useless. Things like

//Class: ThreadPool

don't explain anything – they're right above the line that says class ThreadPool; that is, exactly what your comment said. You should remove them and replace them with comments explaining what bits of complicated code do and why you made unintuitive design decisions.