c++ threading, duplicate/missing threads

Question

I'm trying to write a program that concurrently add and removes items from a "storehouse". I have a "Monitor" class that handles the "storehouse" operations:

class Monitor
{
private:
    mutex m;
    condition_variable cv;
    vector<Storage> S;
    int counter = 0;
    bool busy = false;;
public:

    void add(Computer c, int index) {
        unique_lock <mutex> lock(m);
        if (busy)
            cout << "Thread " << index << ": waiting for !busy " << endl;
        cv.wait(lock, [&] { return !busy; });
        busy = true;
        cout << "Thread " << index << ": Request: add " << c.CPUFrequency << endl;

        for (int i = 0; i < counter; i++) {
            if (S[i].f == c.CPUFrequency) {
                S[i].n++;
                busy = false;   cv.notify_one();
                return;
            }
        }
        Storage s;
        s.f = c.CPUFrequency;
        s.n = 1;
        // put the new item in a sorted position
        S.push_back(s);
        counter++;
        busy = false; cv.notify_one();
    }
}

The threads are created like this:

void doThreadStuff(vector<Computer> P, vector <Storage> R, Monitor &S)
{
    int Pcount = P.size();

    vector<thread> myThreads;
    myThreads.reserve(Pcount);

    for (atomic<size_t> i = 0; i < Pcount; i++)
    {
        int index = i;
        Computer c = P[index];
        myThreads.emplace_back([&] { S.add(c, index); });
    }

    for (size_t i = 0; i < Pcount; i++)
    {
        myThreads[i].join();
    }
// printing results
}

Running the program produced the following results:

I'm familiar with race conditions, but this doesn't look like one to me. My bet would be on something reference related, because in the results we can see that for every "missing thread" (threads 1, 3, 10, 25) I get "duplicate threads" (threads 2, 9, 24, 28).

I have tried to create local variables in functions and loops but it changed nothing.

I have heard about threads sharing memory regions, but my previous work should have produced similar results, so I don't think that's the case here, but feel free to prove me wrong.

I'm using Visual Studio 2017

Answer 1

Here you catch local variables by reference in a loop, they will be destroyed in every turn, causing undefined behavior:

for (atomic<size_t> i = 0; i < Pcount; i++)
{
    int index = i;
    Computer c = P[index];
    myThreads.emplace_back([&] { S.add(c, index); });
}

You should catch index and c by value:

myThreads.emplace_back([&S, index, c] { S.add(c, index); });

Answer 2

Another approach would be to pass S, i and c as arguments instead of capturing them by defining the following non-capturing lambda, th_func:

auto th_func = [](Monitor &S, int index, Computer c){ S.add(c, index); };

This way you have to explicitly wrap the arguments that must be passed by reference to the thread's callable object with std::reference_wrapper by means of the function template std::ref(). In your case, only S:

for (atomic<size_t> i = 0; i < Pcount; i++) {
    int index = i;
    Computer c = P[index];
    myThreads.emplace_back(th_func, std::ref(S), index, c);
}

Failing to wrap with std::reference_wrapper the arguments that must be passed by reference will result in a compile-time error. That is, the following won't compile:

myThreads.emplace_back(th_func, S, index, c); // <-- it should be std::ref(S)

See also this question.