Simplest Mutex ever. Does this example work? Is it thread-safe?

Question

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?

Making a correct mutex mechanism is a complex business. I would use something already out there. — andre, Feb 11 '13 at 18:57
Generally, synchronization primitives such as mutexes require some atomicity (often at the CPU instruction level). Here's an interesting link I found: http://lists.canonical.org/pipermail/kragen-tol/1999-August/000457.html — Tom, Feb 11 '13 at 19:06
The simplest mutex (or synchronization construct in general) is the one you don't need ;-) — , Feb 11 '13 at 19:16

Mike Seymour · Accepted Answer · 2013-02-11T19:07:06.403

9

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.

edited Feb 11 '13 at 19:07

answered Feb 11 '13 at 19:00

Mike Seymour

249,747
28
448
644

hmmmmm. Thanks for the answers. I'm using VS2010 and g++, where the first one doesn't support mutex. Any other compiler independent idea? – The Quantum Physicist Feb 11 '13 at 19:42
1

@SamerAfach: The [Boost.Thread](http://www.boost.org/doc/libs/release/doc/html/thread.html) and [just::thread](http://www.stdthread.co.uk/) libraries have portable and (more or less) direct replacements for the C++11 mutex. Failing that there's the Windows API (critical sections, or whatever they call it); obviously that's not compiler-independent, so you'll need some carefully placed `#define`s to select which to use. – Mike Seymour Feb 11 '13 at 19:48
@MikeSymour if I use the boost Mutex for example (or any other Mutex), will that make the code thread-safe for other parallelization libraries, like OpenMPI and OpemMP? or does Mutex work exclusively for the parallelism of the library from which the Mutex is taken? – The Quantum Physicist Feb 12 '13 at 07:46
1

@SamerAfach: I don't quite understand the question, but I guess the answer is yes. Using a mutex correctly will serialise all sections of code that run with that mutex locked, making that code thread-safe, regardless of whatever parallelisation libraries you're using. – Mike Seymour Feb 14 '13 at 10:58
"regardless of whatever parallelisation libraries you're using". That answers my question. Thanks :) – The Quantum Physicist Feb 14 '13 at 11:03

score 6 · Answer 2 · answered Feb 11 '13 at 18:59

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.

score 6 · Answer 3 · answered Feb 11 '13 at 19:03

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;
}

score 2 · Answer 4 · edited May 23 '17 at 11:48

2

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.

edited May 23 '17 at 11:48

Community

1
1

answered Feb 11 '13 at 18:58

Drew Dormann

59,987
13
123
180

score 2 · Answer 5 · answered Feb 11 '13 at 18:59

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.

score 1 · Answer 6 · answered Feb 11 '13 at 19:05

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.

score 0 · Answer 7 · answered Jan 02 '19 at 09:09

You cannot implement it in C++. You have to use LOCK CMPXCHG. Here is my answer from here:

; BL is the mutex id
; shared_val, a memory address

CMP [shared_val],BL ; Perhaps it is locked to us anyway
JZ .OutLoop2
.Loop1:
CMP [shared_val],0xFF ; Free
JZ .OutLoop1 ; Yes
pause ; equal to rep nop.
JMP .Loop1 ; Else, retry

.OutLoop1:

; Lock is free, grab it
MOV AL,0xFF
LOCK CMPXCHG [shared_val],BL
JNZ .Loop1 ; Write failed

.OutLoop2: ; Lock Acquired

Simplest Mutex ever. Does this example work? Is it thread-safe?

7 Answers7