Simple thread pool in C++

I wrote a simple thread pool, which works pretty well. I would like to see your review of it.

One important feature that I needed in the pool is the ability to wait until all the tasks that I sent to the pool are complete (so that later I could send other tasks that are dependent on the result of the previous tasks).

This is the basic task which you inherit from, to send tasks:

class Task
{
public :
    virtual void ExecuteTask() = 0;
};

This is the .h file of the Thread pool:

#include
#include 
#include

using namespace std;

class ThreadPool
{
public :
    ThreadPool(int threadCount=0);
    bool ScheduleTask(Task* task);
    bool WaitForWorkToBeFinished(DWORD dwMilliseconds);
    ~ThreadPool();

private :
    static DWORD WINAPI ThreadStart(LPVOID threadParameters);
    DWORD ThreadMain();

    list mTasks;
    HANDLE* mThreads;
    int mThreadCount;
    CRITICAL_SECTION mCriticalSection;
    CONDITION_VARIABLE mConditionVariable;
    CONDITION_VARIABLE mConditionVariableTaskFinished;
    int mNumberOfTasksNotFinished;
    bool mConsum;
};

This is the CPP file:

```
#include "ThreadPool.h"

ThreadPool::ThreadPool(int threadCount)
{
mThreadCount=threadCount;
mThreads = new HANDLE[mThreadCount]; //This handles will be used to wait upon them

InitializeCriticalSection(&mCriticalSection);
InitializeConditionVariable(&mConditionVariable);
InitializeConditionVariable(&mConditionVariableTaskFinished);
mConsum = true;
mNumberOfTasksNotFinished = 0;
for(int i = 0; iThreadMain();
}

DWORD ThreadPool::ThreadMain() //The main thread function
{
do
{
Task* currentTask;
EnterCriticalSection(&mCriticalSection);

while(mTasks.size() ==0 && mConsum)
SleepConditionVariableCS(&mConditionVariable,&mCriticalSection, INFINITE);

if(!mConsum) //Destructor ordered on abort
{
LeaveCriticalSection(&mCriticalSection);
return 0;
}

//If we

A few points:

Your EnterCriticalSection/LeaveCriticalSection code should be placed into a scoped RAII object. This saves you from explicitly calling leave, makes your code smaller, and avoids a deadlock if the synchronized code throws.

Do something similar with everything that requires cleanup (like mThreads = new HANDLE[mThreadCount];).

The interface of your class should receive the tasks by reference and store them by pointer within. This way, you make it clear to client code that the class doesn't take ownership of the tasks it receives. (see Third Edit below).

Use reinterpret_cast instead of C-style casts (because C-style casts = bad, mmmkay?!)

using namespace std;

Don't do this. It greatly restricts what client code (whoever includes your class) can write, and the errors generated in client code are almost always obscure. You're better off repeating that in local scopes (if you must) or (better still) not using it (consider writing using std::string; and similar, in the scopes you use the type, or simply declaring std::string x;).

Edit: If bool ThreadPool::ScheduleTask(Task* task) never returns false, why does it return bool? Shouldn't it be void?

Second Edit: Consder using an unsigned int for the number of threads (mThreadCount).

Third edit ( Hi Loki :) )

Regarding ownership of the threads and object lifetime management (how to implement safely and indicate it in the public interface of the class).

Consider this client code:

ThreadPool runner();

// bad bad code: don't call new with raw pointers like this in practice
// but easy to write to make my point below.
BackgroundTask *t = new BackgroundTask(); // class BackgroundTask: public Task
                                          // defined elsewhere

runner.ScheduleTask(t);

What you don't know looking at this code: should you delete t after the ThreadPool completes running? Will runner delete the task itself? If it does, t will point to deleted memory after runner finishes. If it doesn't, you will have to remember to check when runner has finished, and call delete explicitly, every single time you use the thread pool.

You could implement the ThreadPool to delete all tasks after execution, but then, an enterprising programmer (maybe yourself) could write:

ThreadPool runner();
BackgroundTask t();
runner.ScheduleTask(&t); // pass address of automatic storage object

and this will crash your program when your ThreadPool deletes the task.

Another client code alternative, this one much much worse:

void crashy(ThreadPool& pool) {
    BackgroundTask t;
    pool.ScheduleTask(&t);
} // t goes out of scope here and is destroyed but p still holds it's address

ThreadPool p;
crashy(p); // some compilers tend to happily accept this (maybe with a warning)
// p now has a pointer to a deleted BackgroundTask instance

The interface doesn't indicate ownership and makes you interrupt your coding, to look at the implementation, just so you know how to write client code that doesn't leak or crash.

Solutions:

You could change your interface to take a Task& instead. (Similar to passing by pointer), the only drawback is that you need to make sure the tasks you pass to ScheduleTask must still be in scope until the ThreadPool stops running (in other words, you can still write the nice crashy function).

This is better, as at least you don't need to wonder if it will delete the task (which passing by pointer may, or may not suggest).

Better yet, change your interface to take a std::unique_ptr instead:

class ThreadPool
{
public :
    bool ScheduleTask(std::unique_ptr task) // > mTasks; // << notice std::unique_ptr
    ... 
};

Client code:

ThreadPool runner();
std::unique_ptr p(new BackgroundTask());
runner.ScheduleTask(std::move(p)); // works, no lifetime management problems
                                   // and no ambguity either

Since the function takes a unique_ptr now, you now know you need to pass a pointer and you also know you are passing ownership as well.

StackExchange Code Review Q#40536, answer score: 7