1

I wrote some code before but I realized it has a very bad code style so I changed it.

I changed the A.unlock inside of the if block. I know if the if never run then this thread will unlock the mutex which does not belong to itself and then it will return an undefined behavior. My question is, if it returns an undefined behavior, will the logic here still work? Because if thread t1 didn't have the lock, t1 unlock the mutex A will return undefine behavior and the mutex will still be held by the thread which holds it right? And it will not affect the other logic in this code. My old code works as same as I put the unlock part inside of the if block. So that's why I am curious how can this work.

mutex A;
if(something)
{
A.lock();
}
A.unlock();
6
  • "My question is, if it returns an undefined behavior, will the logic here still work?" Do you understand what "undefined behavior" is? If the code exhibits undefined behavior, the standard makes no guarantees on the correctness of your code. It might seem to work, by accident, sure. There are just no guarantees, that it will continue to work. Jul 28, 2019 at 0:09
  • That is what I am thinking. Because before this bad code worked perfectly as what we expected. If there were thousands of bugs we would notice this part of bad code immediately. It seems the code just worked fine.
    – Gavin Xu
    Jul 28, 2019 at 0:12
  • "If there were thousands of bugs we would notice this part of bad code immediately." Code that exhibits undefined behavior is a bug. The only way to avoid it is to just not write the code that exhibits undefined behavior. Since, in theory, there is no way to detect 100% of cases of undefined behavior (static code analyzers can help somewhat, though). Jul 28, 2019 at 0:22
  • Thanks but I am not sure "works" mean here. You mean the code might work or the unlock might work so it might unlock the mutex which is held by another thread?
    – Gavin Xu
    Jul 28, 2019 at 0:34
  • 1
    Possible duplicate of Undefined, unspecified and implementation-defined behavior
    – JaMiT
    Jul 28, 2019 at 3:50

4 Answers 4

2

When calling unlock on a mutex, the mutex must be owned by the current thread or the behavior is undefined. Undefined behavior means anything can happen, including the program appearing to run correctly, the program crashing, or memory elsewhere getting corrupted and a problem not being visible until later.

Normally a mutex is not used directly; one of the other standard classes (like std::unique_lock or std::lock_guard are used to manage it. Then you won't have to worry about unlocking the mutex.

3
  • So it is possible after I ran A.unlock() it unlocked A which is held by another thread?
    – Gavin Xu
    Jul 28, 2019 at 0:22
  • @GavinXu No. If another thread has locked the mutex, trying to unlock it will be Undefined Behavior. Jul 28, 2019 at 0:24
  • So the code I wrote above might cause some memory bug or crash the program directly but not unlock the mutex if the if-else block never run? I think I got the reason why I didn't find this before. if they don't unlock other threads then that is what we expected: if the if-else block runs, then it will be unlocked after the if block. if the block never runs, the undefined behavior will not affect other threads and this thread also doesn't hold the mutex. so our old test cases couldn't find it.
    – Gavin Xu
    Jul 28, 2019 at 0:28
2

You should consider using std::lock_guard :

mutex A;
if (something)
{
    lock_guard<mutex> lock(A);
}
0

It is an undefined behavior to unlock a mutex you haven't locked. It may work on some cases for so many times but some day it will break and behave differently.

You can use a lock_guard in your case and forget about lock/unlock

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

The ISO C++ standard has this to say on the matter, in [thread.mutex.requirements.mutex] (my emphasis):

The expression m.unlock() shall be well-formed and have the following semantics:

Requires: The calling thread shall own the mutex.

That means what you are doing is a violation of the standard and is therefore undefined. It may work, or it may delete all your files while playing the derisive_maniacal_laughter.mp3 file :-)

Bottom line, don't do it.

I also wouldn't put the unlock inside the loop since modern C++ has higher-level mechanisms for dealing with auto-release of resources. In this case, it's a lock guard:

std::mutex mtxProtectData; // probably defined elsewhere (long-lived)
: : :
if (dataNeedsChanging) {
    std::lock_guard<std::mutex> mtxGuard(mtxProtectData);
    // Change data, mutex will unlock at brace below
    //   when mtxGuard goes out of scope.
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.