How to manipulate a vector using threads in a class?

Question

I intend to manipulate a vector using threads inside a class.cpp source code not the main.cpp source code via a class member function.

my main.cpp is:

#include "stdafx.h"
#include <stdlib.h>
#include <vector>
#include "class.h"

using namespace std;
int main()
{
    CLASS* classObject = new CLASS();
    classObject->classMemberFucntion(... some arguments....);
    return 0;
}

class.h is:

#include <vector>

#ifndef SLIC_H
#define SLIC_H

class CLASS
{
    protected:
    std::vector<int> cluster;

    public:

    void classMemberFunction(... some arguemnts ... );

    //creating threads function
    void thread2(std::vector<int> *cluster);
    void thread3(std::vector<int> *cluster);
    void thread4(std::vector<int> *cluster);
};
#endif

and the class.cpp is:

#include "class.h"
#include <vector>
#include <thread>
using namespace std;

void CLASS::classMemberFunction(... some arguments ...)
{
    thread t2(&CLASS::thread2, CLASS(), &cluster);
    t2.join();
    thread t3(&CLASS::thread3, CLASS(), &cluster);
    t3.join();
    thread t4(&CLASS::thread4, CLASS(), &cluster);
    t4.join();

    //main thread===================================================
    for (unsigned n = 0; n < 1000/4; ++n)
            {
                cluster.push_back(-1);
            }
    cout << cluster.size() << endl;
}

void CLASS::thread2(std::vector<int> *cluster)
{
    for (unsigned n = 0; n < 1000 / 4; ++n)
    {
        (*cluster).push_back(-1);
    }
}

void CLASS::thread3(std::vector<int> *cluster)
{
    for (unsigned n = 0; n < 1000 / 4; ++n)
    {
        (*cluster).push_back(-1);
    }
}

void CLASS::thread4(std::vector<int> *cluster)
{
    for (unsigned n = 0; n < 1000 / 4; ++n)
    {
        (*cluster).push_back(-1);
    }
}

as it is seen, I intend to start three threads inside a member function (the member function acting as a main thread and having a total of four threads) and initialize a vector. I expect the program to print a size of 1000 when calling cout << cluster.size() << endl; but every time the program prints a size of 250 ! I'm new to threading and I have no idea if I'm using the right syntax. So let me know what I'm doing wrong.

UPDATE:
so I revised my code like this:

//main.cpp
#include "stdafx.h"
#include <stdlib.h>
#include <vector>
#include "class.h"

using namespace std;
int main()
{
    myCLASS* classObject = new myCLASS();
    classObject->classMemberFunction();
    return 0;
}

and:

//class.h
#include "stdafx.h"
#include <vector>
#include <mutex>

#ifndef SLIC_H
#define SLIC_H

class myCLASS
{
protected:
    std::vector<int> cluster;
    std::mutex cluster_mutex;

public:

    void classMemberFunction();
    //creating threads function
    void thread2();
    void thread3();
    void thread4();
};
#endif

and:

//class.cpp
#include "stdafx.h"
#include "class.h"
#include <iostream>
#include <vector>
#include <thread>
#include <mutex>
using namespace std;

void myCLASS::classMemberFunction(void)
{
    thread t2(&myCLASS::thread2, *this);
    thread t3(&myCLASS::thread3, *this);
    thread t4(&myCLASS::thread4, *this);

    //main thread===================================================
    for (int n = 0; n < 1000 / 4; ++n)
    {
        cluster.push_back(-1);
    }
    t2.join();
    t3.join();
    t4.join();
    cout << cluster.size() << endl;
}

void myCLASS::thread2()
{
    cluster_mutex.lock();
    for (int n = 0; n < 1000 / 4; ++n)
    {
        cluster.push_back(-1);
    }
    cluster_mutex.unlock();
}

void myCLASS::thread3()
{
    cluster_mutex.lock();
    for (int n = 0; n < 1000 / 4; ++n)
    {
        cluster.push_back(-1);
    }
    cluster_mutex.unlock();
}

void myCLASS::thread4()
{
    cluster_mutex.lock();
    for (int n = 0; n < 1000 / 4; ++n)
    {
        cluster.push_back(-1);
    }
    cluster_mutex.unlock();
}

when I run the code in Visual Studio 2013, I get the following:

1>------ Build started: Project: threadTEST, Configuration: Release Win32 ------
1>  class.cpp
1>D:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xlocnum(155): error C2280: 'std::mutex::mutex(const std::mutex &)' : attempting to reference a deleted function
1>          D:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\mutex(113) : see declaration of 'std::mutex::mutex'
1>          This diagnostic occurred in the compiler generated function 'myCLASS::myCLASS(const myCLASS &)'
1>  threadTEST.cpp
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

I think there is something wrong with the default copy constructor !

Answer 1

Ok, here are some things:

1. I noticed this:

   for (unsigned n = 0; n < 1000/4; ++n)
   

   You shouldn't use unsigned unless you're sure it's necessary. Otherwise, just use a normal int.

2. This:

   (*cluster).push_back(-1);
   

   is the same thing as:

   cluster->push_back(-1);
   

3. You're using C++11, so you might as well use a unique_ptr to hold CLASS:

   std::unique_ptr<CLASS> classObject = new CLASS();
   

4. All-caps class names are generally discouraged; that convention is primarily used with macros.

5. You are appending to cluster from multiple different threads and therefore may cause a data race. You should manage access to the vector from the threads via a mutex:

   // in your header file
   std::vector<int> cluster;
   std::mutex cluster_mutex;
   
   // in your thread* functions
   cluster_mutex.lock();
   cluster->push_back(-1);
   cluster_mutex.unlock();
   

6. Why are you doing this:

   thread t2(&CLASS::thread2, CLASS(), &cluster);
   t2.join();
   

   There are a few problems here:

   1. You're giving the thread function a copy of the current class.
   2. You're passing it a member...that...could normally be accessed more simply...
   3. You're joining the thread right after it's created. joining waits for the thread to complete. This kind of kills the whole point of threading: concurrency.

   Here's a better version:

   thread t2(&CLASS::thread2, *this);
   

   Remove the std::vector<int>* argument from your thread* functions and remove the explicit dereferencing of cluster:

    cluster.push_back(-1);
   

7. Define an explicit constructor and copy constructor:

   myCLASS() {}
   myCLASS(myCLASS& rhs): cluster(rhs.cluster) {}
   

Also, when I said "try not to use all-caps," I meant something more like renaming CLASS to maybe Class or class, not myCLASS. Still looks really weird. Of course, at this point its more of a personal taste...but still.

Answer 2

Just to compliment the other great answers, I decided to put together a quick example of writing to a vector from multiple threads without a locking mechanism. Uses C++11 features, which is okay because you are too. :)

//============================================================================
// Name        : HelloThreadedVector.cpp
// Author      : Jesse Nicholson
//============================================================================

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

void fillThreaded(std::vector<std::string>& vec, int startIndex, int count)
{
    int threadNumber = 0;

    if (startIndex == 0)
    {
        threadNumber = 0;
    }
    else
    {
        threadNumber = startIndex / count;
    }

    for (int i = startIndex; i < startIndex + count; ++i)
    {
        vec[i] = std::move(std::string("Filled by thread: " + std::to_string(threadNumber)));
    }
}

int main()
{

    // Get the total number of logical cores supported by the machine
    size_t totalLogicalCores = std::thread::hardware_concurrency();

    //Create the vector we're going to fill
    std::vector<std::string> stringsFromThreads;

    //Call resize to ensure we can access the vector at indices 0-999 using subscript operator []
    stringsFromThreads.resize(1000);

    size_t i = 0;

    //Get how many writes each thread will have to perform for all total threads to fill the vector
    size_t totalWritesPerThread = stringsFromThreads.size() / totalLogicalCores;

    //Create a vector of thread objects so we can easily store all the thread we're making
    std::vector<std::thread> runningThreads;

    //Because I abuse std::move(). Not really necessary, could have just pushed_back
    runningThreads.resize(totalLogicalCores);

    //Spawn each thread, pass the vector by reference, pass the start index and the total count of writes the thread is to perform
    for (i = 0; i < totalLogicalCores; ++i)
    {
        runningThreads[i] = std::move(std::thread(&fillThreaded, std::ref(stringsFromThreads), totalWritesPerThread * i, totalWritesPerThread));
    }

    //Join all of our threads
    for(i = 0; i < totalLogicalCores; ++i)
    {
        runningThreads[i].join();
    }

    runningThreads.clear();

    //Viola!
    for (auto s : stringsFromThreads)
    {
        std::cout << s << std::endl;
    }

    return 0;
}

Answer 3

When you start the thread like this:

thread t2(&CLASS::thread2, CLASS(), &cluster);

you pass a reference to a newly constructed CLASS object (CLASS()), with it's own member cluster. So the threads will push back on a temporary object.

Only the final - your main thread as it's called, is operating on the cluster member of the instance on which the member function is called.

Probably you want to create the thread like this:

thread t2(&CLASS::thread2, *this, &cluster);

because then it refers to the cluster member of the this object.

NOTE Your code is inherently very thread unsafe. The only reason your code currently works is because of the .join() you do before creating the next thread, effectively serializing your code.

As soon as those threads will run in parallel, your code will explode. Segfault, hang, crash & burn. The shared vector<int>, that all threads intend to modify, needs to be protected by a mutex.

In which case you effectively serialize access to the shared variable, rendering the use of threads, in this case, useless and complicates the code to no end.

EDIT

class CLASS
{
    protected:
    std::vector<int> cluster;
    std::mutex       mtx;   // <-- add this
...
};

Then start the threads like so:

thread t2(&CLASS::thread2, *this);

And change the threadN member functions to:

void CLASS::thread2( void ) {
     mtx.lock();
     // ... do things with `cluster`
     mtx.unlock();
}