Class member method based multithreading [C++]

Question

In briefly: I'm working on a little chat program just for fun.

I am using threads to control two main parts of the application. I am making a socket controller class based on the following design:

//this is a "main.cpp example" for the design

std::mutex mtx;

class AThreadedClass
{
public:
    AThreadedClass()
    {
        running = true;
        value1 = 0;
        value2 = 0;
        value3 = 0;
    }

    //first method which is used for threads
    void run()
    {
        {
            std::lock_guard<std::mutex> lck(mtx);
            value1 = 0; value2 = 0; value3 = 0;
        }

        while (running)
        {
            {
                std::lock_guard<std::mutex> lck(mtx);
                value1 += 2; value2 +=1; value3 -= 1;
                if (value1 > 1000000)
                {
                    running = false;
                }
            }
        }
    }


    //second method used as thread
    //this will possible be just a data
    //retrieving function, not necessarly
    //used as thread
    void writeOut()
    {
        while(running)
        {
            std::lock_guard<std::mutex> lck(mtx);
            std::cout << value1 << ';' << value2 << ';' << value3 << '\n';
        }
    }
private:
    int value1, value2, value3;
    std::atomic<bool> running;
};

int main()
{
    AThreadedClass class1;
    std::thread t1(&AThreadedClass::run, &class1);
    std::thread t2(&AThreadedClass::writeOut, &class1);

    t1.join();
    t2.join();

    std::cout << "Threads finished successfully!\n";

    return 0;
}

So, here are the things I was wondering about:

• Is this a good way to manage things? (I mean, using classes and their member functions for threads could be a stupid idea :/)
• Are there possible deadlocks, when I am mixing std::atomic with std::mutex like in the example? (So far it worked perfectly for me)

And if I didn't notice an existing question about this, please point to it.

Answer 1

• Is this a good way to manage things? (I mean, using classes and their member functions for threads could be a stupid idea :/)

No. You don't force any sequencing on the threads.

• Are there possible deadlocks, when I am mixing std::atomic with std::mutex like in the example? (So far it worked perfectly for me)

Yes. Though unlikely.

This loop can lock out the other thread from running completely (because it also tries to lock mtx).

    while(running)
    {
        std::lock_guard<std::mutex> lck(mtx);
        std::cout << value1 << ';' << value2 << ';' << value3 << '\n';
    }

Just because you release the lock does not mean waiting threads are guranteed to be the first thread that is given the lock next. It is just as likely that this thread will regain the lock again when it re-enters the loop.

You should be using condition variables to force an ordering.

Answer 2

I think you have an OK approach, however it's hard to tell if it's going to work for you because at the moment you seem to be at concept phase. As you develop the application, you may find that having different methods for different thread loops may mean your class is responsible for too much. I tend to prefer to have a class for each thread I create, which has a main thread function. An example of which would be the WorkerThread in this question I posted.

Nested scopes in functions

As far as your coding style goes, I'm not super keen on the way you're using braces to create scoping for your mutex uses:

{
    std::lock_guard<std::mutex> lck(mtx);
    value1 = 0; value2 = 0; value3 = 0;
}

I'd be inclined to create a function with a name if I felt like I needed to have the extra scoping and put the body into it. Your run method also re-initializes the values even these have been set in the constructor, it's unclear if this is on purpose or not.

Global Mutex

You've got std::mutex mtx; as a global. If you're only going to be using it from within your AThreadedClass, it would be better as a static class member. I'd also update its name to reflect what it is it's used to lock.

Atomic Vs Volatile

Whilst atomic is the safe option for running, I'm not sure you really need it to be atomic. You never update its value outside of the mtx lock. I would expect that marking it as volatile should be enough to prevent the compiler from optimizing out the checks in the while loops.