CODEWITHSUNDEEP
c++multithreadingvectorthread-safetystd
SouthParkGerman y
SouthParkGerman y

Reputation: 21

No thread-safety between different vectors in C++?

My problem deals with the usage of different vectors at the same time. I know I can't expect the same vector to work in multiple threads simultaneously. I've broken down the program so it is easier to understand it. I have a ThreadClass class that has a constructor which just adds an element to the vector k and then calls a thread toCall which then outputs the size of the vector which is supposed to be one. The object of this class is created inside of a different vector inside of the main() function using vector's push_back member.

The output turns out to be 0. Sometimes I can get 1 as well. I can produce more of the number 1 if I switch to debug mode. I have tested this problem on a gnu C++17 compiler (Ubuntu 16.04) and a Visual Studio compiler (Windows 10). My question is now if this example shows that I should avoid using vectors in multi-threaded programs totally?

class ThreadClass 
{
private:
    std::vector<int> k;
    std::thread thr;
public:
    ThreadClass() {
        k.push_back(27);
        thr = std::thread(&ThreadClass::toCall, this);
    }
    void toCall() {
        std::cout << k.size() << std::endl;
    }
    void close() {
        if (thr.joinable())thr.join();
    }
};

int main(){
    std::vector<ThreadClass> lols;
    lols.push_back(ThreadClass());
    lols[0].close();
    return 0;
}

Upvotes: 2

Views: 75

Answers (2)

hegel5000
hegel5000

Reputation: 884

The problem is that a value of type ThreadClass holds a reference to itself. Specifically, thr contains a copy of this.

When you copy or move such a value, e.g. when the temporary ThreadClass() is moved into lols, the duplicate holds a duplicate this ptr, i.e. it points to the old temporary, whose lifetime ends after the call to lols.push_back finishes.

We can replicate this problem without threads:

class Foo
{
private:
    std::vector<int> k;
    Foo* possibly_this;
public:
    Foo() {
        k.push_back(27);
        possibly_this = this;
    }
    void toCall() {
        std::cout << possibly_this->k.size() << std::endl;
    }
};

int main(){
    std::vector<Foo> lols;
    lols.push_back(Foo{});
    lols[0].toCall();
}

(For me, it prints 0 with -O0 on 7.3.1, but again, it's UB, so it could do anything on your machine.)

lols.emplace() will not help. If a std::vector resizes, then all pointers/iterators into it are invalidated. Unfortunately, you can't change the pointers stored in thr, so you're left with one solution: disable ThreadClass's copy and move constructors, like so:

  //within the definition of ThreadClass
  ThreadClass(ThreadClass const&) = delete;

In order to place ThreadClass in a container, you will need an additional level of indirection to allow the actual object of a value of type ThreadClass to have a stable location. Either std::list<ThreadClass> or std::vector<std::unique_ptr<ThreadClass>> will do the trick.

Upvotes: 2

David Schwartz
David Schwartz

Reputation: 182865

One issue is that your thread can call toCall before the constructor returns. It's not a good idea to go creating threads that call back into the object in the constructor. Defer the thread creation to some kind of start or launch function and call that after the constructor returns.

This is also a problem:

lols.push_back(ThreadClass());

Here, the destructor (of the temporary) can even run before toCall gets called! That's definitely not going to work. That's another really good reason not to create a thread in a constructor -- it makes temporary objects disastrous.

Upvotes: 0

Related Questions