I found a few issues, here they are (in order in which they were found). The list may seem long, but this does not mean that you did a bad job. Instead, it means that the feedback is detailed ;)
If some issue starts with [major], it means that it would stop me from using your class.
- [major] There is zero documentation.
Thread struct is not really a thread. In fact, it just stores a single
std::function member. Why did you name it like that?
Threadpool has a virtual destructor. This is not necessary, because it does not make sense to inherit from it. I suggest you make it non-virtual.
- The prefix
get usually indicates that you return something from it. However,
getThread does not return anything. Even though
t is passed by reference, you do not change it. Maybe
addTask is a better name.
- [major] If the thread pool is full,
getThread blocks until one of the threads is free. This is bad. In addition, you use an active loop to block, which means that waiting takes 100% of one CPU core. This is really bad. The whole point of threads and thread pools is that the user does not need to wait. I suggest that you copy or move the task (your
std::function) into the pool and execute it as soon as some thread is free. This way,
getThread can return immediately.
- [major] The thread pool currently creates one
std::thread for each task that is added. However, creating threads has some overhead and this will make a big difference if you have many small tasks. I suggest that you create
m_capacity worker threads that grab the tasks from some container. This transforms your problem into a typical producer-consumer problem (adding a task is producing work, which is then consumed by the threads).
- [major] You are using
std::function<void(Args...)> to store a single task. This means that the thread pool does not support return values. A common pattern is that
addTask returns a
std::future which will receive the return value after some thread completed the task. Additionally, thread pool users can use the
std::future to check whether the task completed and they can use it to wait for completion.
- [major] The
getThread function uses references of
t. This means that problems will occur if
t is destroyed while the thread is still running. This is bad, because
t is passed by the user, so the pool has no control over its lifetime. The non-existing documentation would have been a good place to mention that
t must not be destroyed before the task is finished :) Typically, you would
std::move the task, which transfers the control to the pool.
- If I understand
m_mutex correctly, it guards
m_threads against race conditions. This means that you want to lock the mutex whenever you use
m_threads. Since reading from a variable while another thread modifies it is undefined behavior, you need to lock the mutex in some more places. There are some usages of
m_thread or of its elements where the mutex is not locked (and I do not mean constructor/destructor).
ThreadPoolElement has a
volatile bool. Depending on compiler and platform,
volatile may do what you think it does. However, you can not rely on that. You should use
Threadpool has a lower case
ThreadPoolElement has an upper case
I suggest that you refactor the
Threadpool such that you do not need to create a new
std::thread object for each task. Typically, a thread pool has a queue with pending tasks and a fixed number of worker threads. The worker threads grab tasks from the queue and complete them. This way, it is possible to add many tasks without the overhead of creating new threads.
You may want to look into the
std::future classes, since they can simplify your implementation. You can probably get rid of the