Bdtjtk

I would like to ask about the simplest Mutex approach ever for multi-threading. Is the following code thread-safe (quick-n-dirty)?

class myclass

{

    bool locked;

    vector<double> vals;



    myclass();

    void add(double val);

};



void myclass::add(double val)

{

    if(!locked)

    {

        this->locked = 1;

        this->vals.push_back(val);

        this->locked = 0;

    }

    else

    {

        this->add(val);

    }

}



int main()

{

    myclass cls;

    //start parallelism

    cls.add(static_cast<double>(rand()));

}

Does this work? Is it thread-safe? I'm just trying to understand how the simplest mutex can be written.

If you have any advice about my example, would be nice.

Thank you.

Thanks for saying that it doesn't work. Can you please suggest a fix which is compiler independent?

Is it thread-safe?

Certainly not. If a thread is preempted between checking and setting the lock, then a second thread could acquire that lock; if control then returns to the first thread, then both will acquire it. (And of course, on a modern processor, two or more cores could be executing the same instructions simultaneously for even more hilarity.)

At the very least, you need an atomic test-and-set operation to implement a lock like this. The C++11 library provides such a thing:

std::atomic_flag locked;



if (!locked.test_and_set()) {

    vals.push_back(val);

    locked.clear();

} else {

    // I don't know exactly what to do here; 

    // but recursively calling add() is a very bad idea.

}

or better yet:

std::mutex mutex;



std::lock_guard<std::mutex> lock(mutex);

vals.push_back(val);

If you have an older implementation, then you'll have to rely on whatever extensions/libraries are available to you, as there was nothing helpful in the language or standard library back then.

No, this is not thread safe. There's a race between

if(!locked)

and

this->locked = 1;

If there is a context switch between these two statements, your lock mechanism falls apart. You need an atomic test and set instruction, or simply use an existing mutex.

This code doesn't provide an atomic modification of vals vector. Consider the following scenario:

//<<< Suppose it's 0

if(!locked)

{   //<<< Thread 0 passes the check

    //<<< Context Switch - and Thread 1 is also there because locked is 0

    this->locked = 1;

    //<<< Now it's possible for one thread to be scheduled when another one is in

    //<<< the middle of modification of the vector

    this->vals.push_back(val);

    this->locked = 0;

}

Does this work? Is it thread-safe?

No. It will fail at times.

Your mutex will only work if other threads never do anything between the execution of these two lines:

if(!locked)

{

    this->locked = 1;

...and you have not ensured that.

To learn about the how of mutex writing, see this SO post.

No, that is not thread safe.

Consider two threads running myclass::add at more-or-less the same time. Also, imagine that the value of .locked is false.

The first thread executes up to and including this line:

if(!locked)

{

Now imagine that the system switches context to the second thread. It also executes up to the same line.

Now we have two different threads, both believing that they have exclusive access, and both inside the !locked condition of the if.

They will both call vals.push_back() at more-or-less the same time.

Boom.

Others have already shown how your mutex can fail, so I won't rehash their points. I will only add one thing: The simplest mutex implementation is a lot more complicated than your code.

If you're interested in the nitty gritty (or even if you are not - this is stuff every software developer should know) you should look at Leslie Lamport's Bakery Algorithm and go from there.