HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple C++ thread pool

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
poolsimplethread

Problem

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 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

/// \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& );

    /// \brief a blocking function that waits until the threads have processed all the tasks in the queue.
    void wait() const;

private:
    std::vector            _workers;
    std::queue>   _taskQueue;
    std::atomic_uint                    _taskCount;
    std::mutex                          _mutex;
    std::condition_variable             _condition;
    std::atomic_bool                    _stop;
};


And my implementation is as follows:

```
#include
#include

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 void {
while (true) {
std:

Solution

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.

Code Snippets

for ( std::thread& w: _workers )

Context

StackExchange Code Review Q#79323, answer score: 5

Revisions (0)

No revisions yet.