Search code examples
c++multithreadingstdvectorstdthread

Attempting to Reference Deleted Constructor with std::thread and std::vector?


I'm attempting to use a C++ class (with constructors & destructors), that has an std::thread field, in an std::vector object. I'm using C++ 17 and Visual Studio 2022 to compile all examples shown below.


Here is a simple minimal example that illustrates my issue:

#include <iostream>
#include <thread>
#include <vector>

class TEST
{
public:
    std::thread t;
    bool done;

    TEST()
    {
        this->done = false;
        this->t = std::thread(&TEST::foo, this);
    }

    ~TEST()
    {
        this->done = true;
        this->t.join();
    }

    void foo()
    {
        std::cout << "foo\n";
        while (!this->done) {};
    }
};

int main()
{
    std::vector<TEST> vec = {};
    vec.push_back(TEST());
    vec.clear();

    return 0;
}

Where I create a vector vec in the main function that contains objects of TEST class, add one TEST object, and then immediately clear the vector. The TEST class contains an std::thread field called t, a bool field called done and a default constructor and destructor. Each created thread in field t just calls the foo function, prints foo to the console, and waits until the done field is set to true by the destructor, and exits. The destructor simply joins the thread.

The specific error I'm prompted with upon a compilation attempt of the above example is as follows:

Error   C2280   'TEST::TEST(const TEST &)': attempting to reference a deleted function  

It is my understanding that the core reason for the appearance of this error is the fact that the std::thread class is intentionally non-copyable, and therefore it's copy constructor was explicitly deleted in the standard library. Hence, when the compiler attempts to find the default copy constructor of my TEST class, it is unable to find std::threads copy constructor, and the compilation fails.

However, what I'm struggling to understand is the fact that this error only appears to occur when the default destructor ~TEST is added by me. Removing it allows the code to compile, albeit the program crashes when ran, I'm assuming because the t thread field doesn't exit properly upon the object's destruction. I don’t understand the reason as to why this error only manifests when the destructor is explicitly added by me, and doesn’t appear with the implicit empty default constructor.


Another thing I noticed is the fact that this code appears to compile & run without issues without the std::vector being used, for instance the following example:

#include <iostream>
#include <thread>

class TEST
{
public:
    std::thread t;
    bool done;

    TEST()
    {
        this->done = false;
        this->t = std::thread(&TEST::foo, this);
    }

    ~TEST()
    {
        this->done = true;
        this->t.join();
    }

    void foo()
    {
        std::cout << "foo\n";
        while (!this->done) {};
    }
};

int main()
{
    {
        TEST test = TEST();
    }

    return 0;
}

Where I just create a single standalone TEST object in the main function, without an std::vector, and immediately destroy it compiles & executes without issues. It appears that the presence of TEST objects in an std::vector simply causes the ~TEST destructor to fail to compile.

I've also tried using the emplace_back function of std::vector instead of push_back, as suggested by another posting, with the same error appearing.


What is going on here? Why is the presence of objects with std::thread fields in an std::vector causing compilation failures, when such standalone objects do not?


Solution

  • As per cppreference, for the move constructor to be implicitly generated for you, there must be

    (...) no user-declared destructor.

    which is not the case in your example. However,

    user may still force the generation of the implicitly declared move constructor with the keyword default.

    So, technically you could do:

    class TEST {
    public:
        bool done;
        std::thread t;
        TEST(const TEST&) = delete;
        TEST(TEST&&)      = default;
    /// rest remains unchanged
    

    Now you will see that there was a very good reason for the compiler to assume that it should not generate the move constructor for you. Trying to

        std::vector<TEST> vec = {};
        vec.push_back(TEST());
        vec.clear();
    

    will cause std::system_error exception, because in your moved-from object you are still calling t.join() on the thread that is not joinable anymore. You could try to fix it by:

        this->done = true;
        if (t.joinable()) {
            this->t.join();
        }
    

    Even if you take care of this, you are still left with undefined behavior, because of your infinite loop without any side effects:

    while (!this->done) {};
    

    You could add a side effect by changing your bool to std::atomic_bool, but atomics are not move-constructible so you would need to write your own TEST move constructor in any case, e.g. using done( other.done.load( ) ).

    EDIT (thanks @Red.Wave)

    Even if you solve the issue with moving the atomics, you still have Undefined Behavior, because your moved thread holds captured this pointer to the moved-from object that no longer exists. Using this pointer, e.g. my calling a member function is UB.

    To conclude, better just let your TEST class to be neither copy, nor move-constructible. You could use a container of smart pointers instead, as mentioned in the answer by @Serge Ballesta.