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

please post a [mre]. As your class doesn't contain any threads then it doesn't need any mutexes, however if your class is called from multiple threads it might need mutexes. — Alan Birtles, Aug 27 '19 at 11:55

score 2 · Answer 1 · answered Aug 27 '19 at 12:05

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.

answered Aug 27 '19 at 12:05

bjhend

1,538
11
25

2

"Always make sure to unlock the mutex before calling cond.notify_all()" - Why? AFAIK its not necessary. – sklott Aug 27 '19 at 12:51
@sklott "The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock." – Oblivion Aug 27 '19 at 12:58
1

"Not need to" and "should not to" is a little bit different, eh? – sklott Aug 27 '19 at 13:02
@sklott yeah the statement is vague in the answer. – Oblivion Aug 27 '19 at 13:17
2

And "_Notifying while under the lock may nevertheless be necessary when precise scheduling of events is required, e.g. if the waiting thread would exit the program if the condition is satisfied, causing destruction of the notifying thread's condition_variable. A spurious wakeup after mutex unlock but before notify would result in notify called on a destroyed object._" - Those spurious wakeups are terrible – Ted Lyngmo Aug 27 '19 at 13:23
1

If I do not lock the mutex, is there some logic error? Can you make an example? THX! – Alfred Aug 27 '19 at 13:26
@Alfred If you do not lock the mutex you may assign a value to `flag` at the same time as another thread is reading `flag` as I explained in my answer. – Ted Lyngmo Aug 27 '19 at 13:47
@Alfred: Imagine your ``flag`` is for example a huge ``vector`` and a function is going to change all elements. Now, when that function has changed half of the elements another function in another thread gets executed and reads half of the old values and half of the new values, which would be inconsistent. The mutex will prevent that. Although it's unlikely that such happens to your ``flag`` it may also happen and would be risky code to not prevent that. – bjhend Aug 27 '19 at 15:16
I got it! Thank you! – Alfred Aug 28 '19 at 13:07
Great note about getting the mutex. Shared variables protected by a mutex must use the mutex on every access, even when it's not apparently needed, as it additionally prevents the compiler and CPU from optimizing out variable reads and cache writebacks to main memory. – Humphrey Winnebago Sep 09 '19 at 02:51
Calling signal outside the critical section is good advice, but may not result in best performance. It theoretically should prevent an extra sleep. However, the OS may attempt to schedule "intelligently". Ending the critical section (by unlocking the mutex) may be perceived as a good opportunity for a context switch. In this case, the thread will be pre-empted before it calls `signal`, resulting in a little longer delay before a waiting thread can run. I would still advise signaling outside the mutex, with the caveat that it is not necessary and not always the best course of action. – Humphrey Winnebago Sep 09 '19 at 02:54

score 1 · Answer 2 · answered Aug 27 '19 at 11:59

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.

answered Aug 27 '19 at 11:59

QCTDev

166
6

I cannot agree. Even if ```printSecond``` is called firstly, it will be blocked until `printFirst ` finished. @QCTDev – Alfred Aug 27 '19 at 13:28
ahh i see the `cond.wait` now. with that you wont need the mutex or the other locks. i would prefer the mutex/lock paradigm since your intent is more obvious. – QCTDev Aug 27 '19 at 14:40
@QCTDev He will still need the mutex to make sure writing to and reading from `flag` doesn't happen at the same time. – Ted Lyngmo Aug 27 '19 at 15:54
@TedLyngmo if it's just `flag` left to protect then `flag` should be made `atomic`. – QCTDev Aug 28 '19 at 07:49
1

@QCTDev That'd introduce one level of locking on top of the already existing mutex that was supposed to protect `flag` that `second` and `third` uses. The correct way to do it would be to use the same locking pattern in `first` as is used in `second` and `third`. – Ted Lyngmo Aug 28 '19 at 08:33
1

@TedLyngmo I'm not saying it isn't. We do agree that the mutex is the optimum solution. – QCTDev Aug 28 '19 at 09:39

score 1 · Answer 3 · answered Aug 27 '19 at 13:41

1

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

answered Aug 27 '19 at 13:41

Ted Lyngmo

93,841
5
60
108

even atomic types exhibit this race condition – Alan Birtles Aug 27 '19 at 18:22
@AlanBirtles I was merely pointing at the race when having simultaneous r/w operations in the same memory location which would lead to UB - and that had it been an atomic, that wouldn't happen. – Ted Lyngmo Aug 27 '19 at 19:01

Is it necessary to lock?

3 Answers3

Further advice