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;
}

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.