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?

#include <utils/Uncopyable.h>
#include <vector>
#include <queue>
#include <mutex>
#include <atomic>
#include <condition_variable>
#include <functional>

/// \brief Use this class to run tasks in parallel.
class ThreadPool : private Uncopyable {
public:

/// This method does nothing if the thread pool is already running,
/// i.e. ThreadPool( size_t ) was called.

/// \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::mutex                          _mutex;
std::condition_variable             _condition;
std::atomic_bool                    _stop;
};


And my implementation is as follows:

#include <utils/ThreadPool.h>
#include <chrono>

:   _workers(),
_mutex(),
_condition(),
_stop( false ) {}

}

_stop = true;
_condition.notify_all();
for ( std::thread& w: _workers ) {
w.join();
}
}

for ( size_t i = 0; i < threads; i++ ) {
_workers.emplace_back( [this]() -> void {
while (true) {
{   //acquire lock
std::unique_lock<std::mutex> lock( _mutex );
_condition.wait( lock, [this]() -> bool {
});

if ( _stop && _taskQueue.empty() ) {
return;
}

}   //release lock
}   //while
});
}   //for
}

{
std::unique_lock<std::mutex> lock( _mutex );
}
_condition.notify_one();
}

while ( _taskCount != 0u ) {
}
}


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.

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.

• 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). – Nelarius Feb 3 '15 at 8:19