patterncppMinor
Simple C++ thread pool
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:
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:
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
-
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
-
I would name
-
In the implementation of
-
You could go for an
to facilitate maintenance if you ever change the contents of
-
For absolute consistency,
-
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.