1

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?

2
  • I'd stop and think about what TEST should support. Should we be able to copy that? No. To move that? Well... still no? If we allow TEST to be moved, the this pointer captured by the thread of one object points to another object, and if the latter gets deleted... UB. Without being able to copy or to (noexcept) move, std::vector can not work. This class needs to be rethinked, or the thread needs to way to update the captured pointer when the underlying TEST object gets moved. Commented Sep 25, 2024 at 11:57
  • Side note: your boolean flag is not synchronized, so the entire program has UB. Commented Sep 25, 2024 at 12:01

3 Answers 3

3

This could be a nice use case for a vector of unique_ptr. That way the object itself is neither moved nor copied and you only handle smart pointers to it:

...
std::vector<std::unique_ptr<TEST>> vec = {};
vec.push_back(std::make_unique<TEST>());
...
Sign up to request clarification or add additional context in comments.

Comments

2

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.

1 Comment

As mentioned in other comments and answers. There's still another UB. The moved thread has captured a pointer to source object, which is both temporary and moved from.
0

As you declared destructor of your class compiler doesn't generate a move and copy constructors. To make std::vector::emplace_back work your class must be movable, at least. To fix it, just add these declarations to your class:

TEST(TEST&& other) = default;
TEST& operator=(TEST&& other) = default;

2 Comments

I don't think that's enough. See my above comment about the thread-captured this.
@chi You are right, but that's another question, I think.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.