4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – k_kaz Nov 14 '16 at 10:19
6
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.