simple ThreadPool implementations

I would like to ask you for a code review of my c++11 Thread Pool implementation. Your constructive criticism is welcome! Could you give me some ideas how to extend it?

The main idea is to synchronise access to tasks with condition variable.

ThreadPoolExecutor.cpp

#include "ThreadPoolExecutor.h"
#include 
#include 

namespace ThreadPool
{
    ThreadPoolExecutor::ThreadPoolExecutor(int maxWorkers)
    {
        this->maxWorkers = maxWorkers;
    }

    ThreadPoolExecutor::~ThreadPoolExecutor()
    {
        this->poolManager.join();
    }

    void ThreadPoolExecutor::ScheduleTask(std::function task)
    {
        std::unique_lock lock(taskQueueMutex);
        bool wasEmpty = this->taskQueue.empty();
        this->taskQueue.push(task);
        if (wasEmpty)
        {
            this->notEmptyTaskQueue.notify_one();
        }

        lock.unlock();
    }

    void ThreadPoolExecutor::Run()
    {
        this->poolManager = std::thread(&ThreadPoolExecutor::ManagePool, this);
    }

    void ThreadPoolExecutor::ManagePool()
    {
        for (int i = 0; i maxWorkers; i++)
        {
            this->workers.push_back(std::thread(&ThreadPoolExecutor::Worker, this));
        }

        for (auto &thread : this->workers)
        {
            thread.join();
        }
    }

    void ThreadPoolExecutor::Worker()
    {
#ifdef _DEBUG
        std::cout  lock(taskQueueMutex);
            if (this->taskQueue.empty())
            {
                this->notEmptyTaskQueue.wait(lock);
            }

            auto task = this->taskQueue.front();
            this->taskQueue.pop();
            lock.unlock();
            task();
        }
    }
}

ThreadPoolExecutor.h

```
#ifndef THREADPOOLEXECUTOR_H_
#define THREADPOOLEXECUTOR_H_

#include
#include
#include
#include
#include "Task.h"
#include

namespace ThreadPool
{
class ThreadPoolExecutor
{
public:
ThreadPoolExecutor(int maxWorkers);
~ThreadPoolExecutor();
void ScheduleTask(s

ThreadManager.h

What is Task.h? Why are you including it? I see no names that aren't covered by the standard headers you include.

There's no apparent order to your private members. You mix methods and data members; it looks like the members are added in the order they were written, as opposed to some logical order. I recommend putting your methods before your data members (my recommendation matches the Google C++ Style Guide), and determining a logical order within each set.

In general, the header is the place to document your interface. What are the semantics of ScheduleTask? Can it be called from multiple threads without mutual exclusion? Will Run return? Does ThreadPoolExecutor reuse threads after there work is complete? If so, will ThreadPoolExecutor ever destroy threads after creating them? Does the destructor block?

Worker is an odd name for a method. It is the usual practice that function names should be some sort of verb phrase; Worker is a noun, and it doesn't give a lot of insight into what it does.

maxWorkers should probably be declared const; it shouldn't change after construction.

Comments on the private data members should clearly indicate which members are guarded by the mutex.

ThreadManager.cpp

Constructors should use initializers wherever possible:

ThreadPoolExecutor::ThreadPoolExecutor(int maxWorkers) : maxWorkers(maxWorkers) {}

This will allow the member to be declared const.

Your destructor throws if Run has never been called: A default constructed thread is not joinable. That's not the biggest problem: Your destructor never returns if maxWorkers > 0 because none of your calls to join ever complete.

It is distracting (and unidiomatic!) to see unnecessary this-> warts prepended to each member access. If you must have a visual indicator that you are accessing members rather than local variables, adopt a naming convention for your data members (common ones I've seen: maxWorkers_, m_maxWorkers).

In ScheduleTask, it is unnecessary to call lock.unlock();. std::unique_lock unlocks its owned mutex in its destructor; when I see explicit unlocking, I expect that it's because of complicated locking semantics (e.g. conditional unlocking, or unlocking before lock goes out of scope, as in Worker) and look around for the complication; there's no such complication here.

maxWorkers appears to be a misnomer as you never have fewer workers. Maybe numWorkers?

StackExchange Code Review Q#37878, answer score: 4