Is it necessary to lock?

Question

I met a question in leetcode. I have viewed some solutions in the discuss. But my solution is different from others because I do not use the lock in the method first. I wonder whether my code is correct. Besides, can you give me some advice about my code?

I think it is not necessary to use unique_lock in method void first(function<void()> printFirst) like void second(function<void()> printSecond), is it right?

class Foo {
public:
    Foo() {

    }

    void first(function<void()> printFirst) {
        // cout<<1<<endl;
        // printFirst() outputs "first". Do not change or remove this line.
        // mtx.lock();
        printFirst();
        flag=1;
        // mtx.unlock();
        cond.notify_all();
        // cout<<11<<endl;
    }

    void second(function<void()> printSecond) {
        // cout<<2<<endl;
        {
            unique_lock<mutex> lk(mtx);
            cond.wait(lk,[this](){return flag==1;});
            // printSecond() outputs "second". Do not change or remove this line.
            printSecond();
            flag=2;
        }

        // cout<<22<<endl;
        cond.notify_all();
    }

    void third(function<void()> printThird) {
        // cout<<3<<endl;
        unique_lock<mutex> lk(mtx);
        cond.wait(lk,[this](){return flag==2;});
        // printThird() outputs "third". Do not change or remove this line.
        printThird();
        flag=3;
        // cout<<33<<endl;
    }

    mutex mtx;
    condition_variable cond;
    int flag=0;
};

Answer 1

It is necessary.

That your code produced correct output in this instance is besides the point. It is entirely possibly that printFirst might not be complete by the timeprintSecond is called. You need the mutex to prevent this and stop printSecond and printThird. from being ran at the same time.

Answer 2

Obviously your three element functions are supposed to be called by different threads. Thus you need to lock the mutex in each thread to protect the common variable flag from concurrent access. So you should uncomment mtx.lock() and mtx.unlock() in first to protect it there as well. Functions second and third apply a unique_lock as an alternative for that.

Always make sure to unlock the mutex before calling cond.notify_all() either by calling mtx.unlock() before or making the unique_lock a local variable of an inner code block as in second.

Further advice

Put a private: before the element variables at the bottom of your class definition to protect them from outside access. That will ensure that flag cannot be altered without locking the mutex.

Answer 3

The flag checking condition in second() or third() may be evaluated at the same time as first() assigns 1 to flag.

Rewrite this

cond.wait(lk, [this](){return flag==1;});

like this and it may be easier to see:

while(!(flag==1)) cond.wait(lk);

It does the same thing as your wait() with a lambda.

flag is supposed to be read while the mutex is held - but first does not care about mutexes and assigns to flag whenever it pleases. For non-atomic types this is a disaster. It may work 10000000 times (and probably will) - but when things actually happen at the same time (because you let it) - boom - Undefined Behaviour