4
\$\begingroup\$

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 <algorithm>
#include <iostream>

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

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

    void ThreadPoolExecutor::ScheduleTask(std::function<void()> task)
    {
        std::unique_lock<std::mutex> 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 < this->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 << "Hello from thread: " << std::this_thread::get_id() << std::endl;
#endif
        while (1)
        {
            std::unique_lock<std::mutex> 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 <functional>
#include <queue>
#include <thread>
#include <memory>
#include "Task.h"
#include <condition_variable>

namespace ThreadPool
{
    class ThreadPoolExecutor
    {
    public:
        ThreadPoolExecutor(int maxWorkers);
        ~ThreadPoolExecutor();
        void ScheduleTask(std::function<void()> task);
        void Run();

    private:
        int maxWorkers;
        std::vector<std::thread> workers;
        std::thread poolManager;
        std::queue<std::function<void()>> taskQueue;
        std::condition_variable notEmptyTaskQueue;
        void ManagePool();
        std::mutex taskQueueMutex;
        void Worker();
    };
}
#endif

And finally usage.

Program.cs

#include "ThreadPoolExecutor.h"
#include "ExampleTask.h"
#include <iostream>

using namespace ThreadPool;

int main()
{
    ThreadPoolExecutor executor = ThreadPoolExecutor(4);

    for (int i = 0; i < 10; i++)
    {
        int random = std::rand() % 6;
        std::chrono::seconds timeout(random);
        executor.ScheduleTask([i, timeout](){
            std::cout << "no " << i << std::endl;
            std::this_thread::sleep_for(timeout);
        });
    }

    executor.Run();

    ExampleTask task;
    executor.ScheduleTask(task);
    std::this_thread::sleep_for(std::chrono::seconds(15));

    for (int i = 0; i < 10; i++)
    {
        int random = std::rand() % 6;
        std::chrono::seconds timeout(random);
        executor.ScheduleTask([i, timeout](){
            std::cout << "no " << i << std::endl;
            std::this_thread::sleep_for(timeout);
        });
    }

    return 0;
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

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?

\$\endgroup\$
10
  • 1
    \$\begingroup\$ I agree with your recomendations. But I don't think quoting google c++ style guide is good. The google C++ style guide is designed for google (and to support there legacy C++ systems). It is not considered a generically good C++ style guide. \$\endgroup\$ Dec 21, 2013 at 23:49
  • \$\begingroup\$ @LokiAstari The Google C++ style guide has its plusses and minuses, but I don't think it's clearly bad; certainly its advice on how to organize code is fine, even if it is a bit old-fashioned when it comes to exceptions and functional style. \$\endgroup\$
    – ruds
    Dec 22, 2013 at 6:10
  • 1
    \$\begingroup\$ I disagree. There are so many bad things in there (I did notice that it has been updated). But just a quick glance Doing Work in Constructors. Advice: don't do it because if things go wrong you have no object to check error codes from. So basically that is suggesting two phase initialization. I am sure I could find lots more. I consider this a travesty of style guide for modern C++. It is absolutely fine for internal google projects because they have a specific goal. \$\endgroup\$ Dec 22, 2013 at 10:11
  • \$\begingroup\$ Yep, there is unnecessary "Task.h" include in ThreadPoolExecutor.h. Thank you for google c++ style, I'll familiarize with it. I agree, that the order is the order of writing code. \$\endgroup\$
    – klouch
    Dec 22, 2013 at 12:08
  • 1
    \$\begingroup\$ @klouch maxWorkers>0 problem can be addressed in a number of ways. One way would be to have a state variable (boolean or preferably an enum) as a class member. Your Worker function, on waking up, could check whether the state is SHUTDOWN, and stop checking the queue in that case. In your destructor, you could set the state and call notify_all on your condition variable before joining with the manager thread. \$\endgroup\$
    – ruds
    Dec 22, 2013 at 17:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.