# v2 of this question is here and v3 is here

To get a better understanding of C++11/C++14, I thought I would develop a thread pool, even if it has been done to death!

The only dependency outside of the standard library is boost::lockfree::queue.

#ifndef THREADPOOL_H

#include <atomic>
#include <functional>
#include <future>
#include <vector>

#include <boost/lockfree/queue.hpp>

{
public:
//  constructors
//
//
//  calls threadpool(size_t concurrency, size_t queue_size) with:
//
//  concurrency - concurrency
//  queue_size  - 128, arbitary value, should be sufficient for most
//                use cases.
//  a maximum number of queued tasks.
//
//  Argument
//    concurrency - the guaranteed number of threads used in the
//                  on concurrently.
//    queue_size  - the maximum number of tasks that can be queued
//                  for completion, currently running tasks do not
//                  count towards this total.

//  destructor
//
//  Will complete any currently running task as normal, then
//  signal to any other tasks that they were not able to run
//  through a std::runtime_error exception

//  run
//
//  Runs the given function on one of the thread pool
//  threads in First In First Out (FIFO) order
//
//  Argument
//    task - function or functor to be called on the
//
//  Result
//    signals when the task has completed with either
//    success or an exception. Also results in an
//    exception if the thread pool is destroyed before
//    execution has begun.

private:
{
public:
std::promise<void> completion_promise;
};

//  Have to use 'task_package *' since a trivial destructor is
//  do not satisfy.
std::atomic<bool> shutdown_flag;

};

#endif


#include "threadpool.hpp"

#include <algorithm>
#include <exception>
#include <utility>

template<typename T>
constexpr T zero(T)
{
return 0;
}

{ };

{ };

shutdown_flag(false),
{
// This is more efficient than creating the 'threads' vector with
// size constructor and populating with std::generate since
// std::thread objects will be constructed only to be replaced

for (auto a = zero(concurrency); a < concurrency; ++a)
{
// emplace_back so thread is constructed in place
{
// checks whether parent threadpool is being destroyed,
// if it is, stop running.
{

// use pop_task so we only ever have one reference to the
{
try
{
}
catch (...)
{
// try and tell the owner that something bad has happened...
try
{
// ...but this can also throw, so stay protected
}
catch (...) { }
}
}
else
{
// rather than spinning, give up thread time to other things
}
}
});

}
};

{
// signal that threads should not perform any new work
shutdown_flag.store(true);

// wait for work to complete then destroy thread
{
}

// signal to each uncomplete task that it will not complete due to
{
try
{
}
catch (...) { }
}
};

{
auto promise = std::promise<void>{};
auto future = promise.get_future();

// ensures no memory leak if push throws (it shouldn't but to be safe)

package->completion_promise = std::move(promise);

// no longer in danger, can revoke ownership so
// tasks is not left with dangling reference
package.release();

return future;
};

{

{
return true;
}
return false;
}


Are there any issues or improvements that you can see?

This does not really stop them spinning:

// rather than spinning, give up thread time to other things


It may give up the core for another thread (temporarily). But it will come back just as quickly to check for more work. So I am not convinced this actually buys you anything.

I would use a condition variable and block threads on it while there is no work. Then you know they are not taking up any cycles when there is no work.

When creating the work package.

package->task = task;


I would forward the task (remember that named variables can not activate move semantics).

package->task = std::forward<std::function<void()>>(task);

• Interesting, I will have to do a couple tests on using and not using yield. With respect to the std::forward, I get a compile error when using clang-503.0.40, threadpool.cpp:108:21: error: no matching function for call to 'forward' considering this would it be better to have run task task by value then std::move it into the package? – Tom Myles Jun 21 '14 at 23:19
• Update the std::forward(). Should have been: std::forward<std::function<void()>>() – Martin York Jun 22 '14 at 1:14
• Ok, I've had a crack at implementing your suggestions, you can see the results here (hint: it is broken!) – Tom Myles Jun 22 '14 at 11:33
• @TomMyles: Added answer that should fix your problem. – Martin York Jun 22 '14 at 17:09