HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Class member method based multithreading [C++]

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methodmemberbasedmultithreadingclass

Problem

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 lck(mtx);
            value1 = 0; value2 = 0; value3 = 0;
        }

        while (running)
        {
            {
                std::lock_guard 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 lck(mtx);
            std::cout  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.

Solution

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 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.

Code Snippets

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

Context

StackExchange Code Review Q#136232, answer score: 3

Revisions (0)

No revisions yet.