Thread Pool Class

Question

I have a thread_pool class, that mimics std::thread. (I would have liked std to have a pool, but alas that is not the case.)

thread_pool.h

#ifndef _C9Y_THREAD_POOL_H_
#define _C9Y_THREAD_POOL_H_

#include <functional>
#include <thread>
#include <vector>

#include "defines.h"

namespace c9y
{
    //! Thread Pool
    class C9Y_EXPORT thread_pool
    {
    public:
        //! Contruct empty thread pool.
        //!
        //! The default constructor will create a thread pool, but with no threads.
        //! It can be used to create an instance on the stack that can later accept
        //! a running thread pool with move assignment.
        thread_pool() noexcept;

        //! Create a thread pool.
        //!
        //! @param thread_func The thread function each thread in the pool executes.
        //! @param concurency The number of threads to run concurently.
        //!
        //! This constructor will create @arg concurency threads that execute @arg thread_func.
        thread_pool(std::function<void ()> thread_func, size_t concurency);

        //! Move Constructor
        thread_pool(thread_pool&& other) noexcept;

        //! Destructor
        //!
        //! @warning If the thread pool is not joined, the underlying std::thread
        //! objects will invoke std::terminate.
        ~thread_pool();

        //! Move Asignment
        thread_pool& operator = (thread_pool&& other) noexcept;

        //! Get the concurency of the thread pool.
        size_t get_concurency() const;

        //! Join each thread in the pool.
        //!
        //! Calling join ensures that each thread function terminates and
        //! the coresponsing memory is freed.
        void join();

        //!
        //! Swap thread pool.
        void swap(thread_pool& other) noexcept;

    private:
        std::vector<std::thread> threads;

        thread_pool(const thread_pool&) = delete;
        thread_pool& operator = (const thread_pool&) = delete;
    };
}

#endif

thread_pool.cpp:

#include "thread_pool.h"

namespace c9y
{
    thread_pool::thread_pool() noexcept {}

    thread_pool::thread_pool(std::function<void()> thread_func, size_t concurency)
    {
        for (size_t i = 0; i < concurency; i++)
        {
            threads.emplace_back(std::thread(thread_func));
        }
    }

    thread_pool::thread_pool(thread_pool&& other) noexcept
    {
        swap(other);
    }

    thread_pool::~thread_pool() {}

    thread_pool& thread_pool::operator = (thread_pool&& other) noexcept
    {
        swap(other);
        return *this;
    }

    size_t thread_pool::get_concurency() const
    {
        return threads.size();
    }

    void thread_pool::join()
    {
        for (auto& thread : threads)
        {
            thread.join();
        }
        threads.clear();
    }

    void thread_pool::swap(thread_pool& other) noexcept
    {
        threads.swap(other.threads);
    }
}

thread_pool_test.cpp

#include <c9y/thread_pool.h>

#include <atomic>
#include <gtest/gtest.h>

TEST(thread_pool, create)
{
    auto count = std::atomic<unsigned int>{0};

    auto pool = c9y::thread_pool{[&]() {
        count++;
    }, 2};
    pool.join();

    EXPECT_EQ(2, static_cast<unsigned int>(count));
}

TEST(thread_pool, default_contructor)
{
    auto pool = c9y::thread_pool{};
}

TEST(thread_pool, move)
{
    auto count = std::atomic<unsigned int>{0};
    auto pool = c9y::thread_pool{};

    pool = c9y::thread_pool{[&]() {
        count++;
    }, 2};
    pool.join();

    EXPECT_EQ(2, static_cast<unsigned int>(count));
}

This code was originally written against C++11 but the library recently was updated to C++20 and I am in the process up "updating" the code; so please consider it in the light of C++20.

Answer 1

Use std::jthread

Since you target C++20, use std::jthread instead of std::thread. Its destructor will automatically call join() so you don't have to, and more importantly so you cannot forget to do so like you did in your destructor. Furthermore, it provides a framework for notifying the thread that it should stop.

If you do use this, it would be nice to manage a stop token in your class thread_pool, and provide a request_stop() member function that will in turn request all threads to stop.

No need to write your own move constructor

Since the vector threads can be move constructed, the compiler can generate an implicit move constructor for your class that does what you want. So you can omit your explicit move constructor.

In fact, you can then omit swap() as well, as std::swap() will then work on your class because it will have a move constructor.

No need to delete the copy constructor

You don't need to delete the copy constructor since threads is already non-copyable.

Provide a default value for concurrency

Often you want your thread pool to have as many threads as your CPU has hardware threads. You can get that number by calling std::thread::hardware_concurrency(). Consider making the parameter concurrency default to that so the user doesn't have to specify it themselves.

Incomplete test suite

You only have a few tests, and they don't capture some of the problems your class has. For example, you don't have a test that creates a thread pool with a given number of threads that ends without calling join(), you never call get_concurrency() or swap(), and you do move-assign but you don't move-construct. What if you construct a thread pool with a thread function but a concurrency of zero?

Move assignment issues

Move assignment looks very safe if the thread_pool moved into did not have any threads, but what if it does? The swap() will exchange them with the thread_pool object being assigned from, but then what? Should join() be called on that object? What if join() was already called? Using std::jthread makes this a lot safer, but even then you might wonder if moving is ever the right thing to do here if the object moved into has one or more (unjoined) threads. Maybe only allow this if there are no threads or if they have all been joined already?

More optimal use of emplace_back()

The great thing about emplace_back() is that it forwards its arguments to the constructor of the value_type of the container. So you can just write:

threads.emplace_back(thread_func);

This avoids creating a temporary std::thread which then is moved into the container.

Incomplete Doxygen documentation

It's great to see the API being documented using Doxygen. However, you did not document all parameters and return values. For example, @return is missing for get_concurency() (which is misspelled: consider using codespell), and @param other is missing for swap().

You can enable warnings in Doxygen that will alert you to these issues. Make sure you run Doxygen now and then on your code, or even better run it automatically as part of your build system.