5

The following code reliably produces a segfault.

#include <vector>
#include <thread>

class Class {
public:
    Class(const int integer)
        : cinteger(integer), carray(std::array<int, 1>{0})
    {}

   const int operator()() 
   {
    carray[cinteger] = 0;
        return cinteger;
    }
    
private:
    int cinteger;
    std::array<int, 1> carray;
};

class Worker{
    public:
        Worker( Class iClass): 
            wClass(iClass),
            thread(&Worker::thread_main, this)
            {}
        
        void join(){thread.join();}     

    private:
        Class wClass;
        std::thread thread;
        void thread_main(){
            for(int i = 0; i < 50000; i++)
            wClass();
        }
};

int main()
{
    std::vector<Worker> Workers;
    for(int i = 0; i < 4; i++)
        Workers.emplace_back(Class(0));
    for(int i = 0; i < 4; i++)
        Workers[i].join();
    return 0;
}

For some reason, I do not understand, the cinteger variable of Class seems to get changed unexpectedly. This problem arises every time I run the code. It does not arise if I only generate 3 workers. Also, at 1000 iterations in the Worker class the problem does not appear.

TBH I am a bit out of ideas of where this problem could come from.

Jabberwocky
  • 48,281
  • 17
  • 65
  • 115
Takirion
  • 173
  • 4
  • 1
    the array of size one was just to reliably throw an exception when cinteger gets changed. For me it compiled without the include, but this is probably luck from my implementation of the standard library. – Takirion Apr 11 '23 at 09:53

2 Answers2

11

This is quite simple really. In the thread you reference this, however as you perform emplace_back in the vector, it'll eventually lead to resize of the vector and move the objects to a different place in memory. Invalidating previous instances. Which leads to this being invalid and corrupted inside the thread_main() member function.

ALX23z
  • 4,456
  • 1
  • 11
  • 18
  • 6
    Good catch, +1. You might want to add a possible solution, like calling `Workers.reserve(4);` before the `push_back`s. – wohlstad Apr 11 '23 at 09:49
5

In addition to the reason provided by the other answer, one easy way to solve this would be to simply dynamically allocate the Worker class. Replace the vector<Worker> by a std::vector<std::unique_ptr<Worker>>, and fix the insertions accordingly.

This would make your worker insensitive to reallocations caused by the vector resizes, which is more robust than to call .reserve as you do not need to know the amount of workers ahead of time.
This is because the pointed Worker will stay where it is at it would be dynamically allocated and does not live within the vector.

If you do know, then you can use the .reserve solution.

asu
  • 1,875
  • 17
  • 27
  • As i actually do know the number i will stick to the .reserve solution but thanks for your answer anyway. – Takirion Apr 11 '23 at 09:56
  • 2
    @Takirion well, as long as you write something simple for yourself, it is fine. But in general, your class `Worker` should be declared as non-movable and use some other classes to store it - that never reallocate, like `std::list`, or vector of unique pointers as suggested. – ALX23z Apr 11 '23 at 10:06