Simple C++ thread pool

Question

I wrote a minimalistic thread pool using some C++11 features. I've used it on two projects so far and it has worked fine, but when I ran the code on my laptop, I believe that my thread pool might be entering some kind of dead-locked state -- occasionally the CPU activity drops to almost nothing, and the program hangs.

What are your opinions on the code?

My header file:

#include <utils/Uncopyable.h>
#include <vector>
#include <queue>
#include <thread>
#include <mutex>
#include <atomic>
#include <condition_variable>
#include <functional>
/// \brief Use this class to run tasks in parallel.
class ThreadPool : private Uncopyable {
public:
 ThreadPool();
 ThreadPool( size_t threads );
 ~ThreadPool();
 /// \brief Initialize the ThreadPool with a number of threads.
 /// This method does nothing if the thread pool is already running,
 /// i.e. ThreadPool( size_t ) was called.
 void initializeWithThreads( size_t threads );
 /// \brief Schedule a task to be executed by a thread immediately.
 void schedule( const std::function<void()>& );
 /// \brief a blocking function that waits until the threads have processed all the tasks in the queue.
 void wait() const;
private:
 std::vector<std::thread> _workers;
 std::queue<std::function<void()>> _taskQueue;
 std::atomic_uint _taskCount;
 std::mutex _mutex;
 std::condition_variable _condition;
 std::atomic_bool _stop;
};

And my implementation is as follows:

#include <utils/ThreadPool.h>
#include <chrono>
ThreadPool::ThreadPool()
 : _workers(),
 _taskQueue(),
 _taskCount( 0u ),
 _mutex(),
 _condition(),
 _stop( false ) {}
ThreadPool::ThreadPool( size_t threads ) : ThreadPool() {
 initializeWithThreads( threads );
}
ThreadPool::~ThreadPool() {
 _stop = true;
 _condition.notify_all();
 for ( std::thread& w: _workers ) {
 w.join();
 }
}
void ThreadPool::initializeWithThreads( size_t threads ) {
 for ( size_t i = 0; i < threads; i++ ) {
 //each thread executes this lambda
 _workers.emplace_back( [this]() -> void {
 while (true) {
 std::function<void()> task;
 { //acquire lock
 std::unique_lock<std::mutex> lock( _mutex );
 _condition.wait( lock, [this]() -> bool {
 return !_taskQueue.empty() || _stop;
 });
 if ( _stop && _taskQueue.empty() ) {
 return;
 }
 task = std::move( _taskQueue.front() );
 _taskQueue.pop();
 } //release lock
 task();
 _taskCount--;
 } //while
 });
 } //for
}
void ThreadPool::schedule( const std::function<void()>& task ) {
 {
 std::unique_lock<std::mutex> lock( _mutex );
 _taskQueue.push( task );
 }
 _taskCount++;
 _condition.notify_one();
}
void ThreadPool::wait() const {
 while ( _taskCount != 0u ) {
 std::this_thread::sleep_for( std::chrono::microseconds(1) );
 }
}

Answer 1

A few minor points, mostly related to coding style and practices:

• The non-copyable pattern is somewhat outdated since C++11 deleteable methods. Some might also argue that explicitly deleting the copy constructor and assignment operator is more clear too. Refer to this SO question.

• Names starting with an underscore are somewhat contrived in the C++ language. For class members, this naming convention is not ilegal, but you might want to consider other options, such as the m_ prefix or an underscore at the end instead. Refer to this SO question for more on the subject.

• I would name wait() as waitAll() or similar, to make it explicit that it will wait for all current tasks to complete.

• In the implementation of ThreadPool::wait(), you should probably be using std::this_thread::yield instead of sleep_for.

• You could go for an auto & in this loop:

   for ( std::thread& w: _workers ) 
  

  to facilitate maintenance if you ever change the contents of _workers by some other type.

• For absolute consistency, size_t is a member of namespace std, so it should be used as std::size_t.

Answer 2

Your code looks OK, the only thing which may cause trouble is usage of the same mutex for queue locking and notification: it will be locked during _condition.wait and at the same time you are trying to lock it to add new task in the queue. Try to use separate mutex for condition and lock queue mutex in initializeWithThreads after condition is fulfilled.

• Nelarius: Good point. I switched to a double-lock queue, and got entirely rid of the lock in the schedule-method. The occasional hang-up seems to persist, however, when using the minGW-w64 compiler, so the problem is somewhere else (I have no issues on linux).