CODEWITHSUNDEEP
c++mutexcocos2d-x
glo
glo

Reputation: 1428

mutex lock is not unlocking

I use a mutex to lock and unlock a variable as I call getter from main thread continuously in the update cycle and I call setter from another thread. I provided the code for setter and getter below

Definition

bool _flag;
System::Mutex m_flag;

Calls

#define LOCK(MUTEX_VAR) MUTEX_VAR.Lock();
#define UNLOCK(MUTEX_VAR) MUTEX_VAR.Unlock();

void LoadingScreen::SetFlag(bool value)
{
    LOCK(m_flag);
    _flag = value;
    UNLOCK(m_flag);
}

bool LoadingScreen::GetFlag()
{
    LOCK(m_flag);
    bool value = _flag;
    UNLOCK(m_flag);

    return value;
}

This works well half the time, but at times the variable gets locked on calling SetFlag and hence it is never set thereby disturbing the flow of code.

Can anyone tell me how to solve this issue?

EDIT:

This is the workaround i finally did. This is just a temporary solution. If anyone has a better answer please let me know.

bool _flag;
bool accessingFlag = false;

void LoadingScreen::SetFlag(bool value)
{
    if(!accessingFlag)
    {
        _flag = value;
    }
}

bool LoadingScreen::GetFlag()
{
    accessingFlag = true;
    bool value = _flag;
    accessingFlag = false;

    return value;
}

Upvotes: 3

Views: 2918

Answers (6)

artless-noise-bye-due2AI
artless-noise-bye-due2AI

Reputation: 22420

The issue you have (which user1192878 alludes to) is due to delayed compiler load/stores. You need to use memory barriers to implement the code. You may declare the volatile bool _flag;. But this is not needed with compiler memory barriers for a single CPU system. Hardware barriers (just below in the Wikipedia link) are needed for multi-cpu solutions; the hardware barrier's ensure the local processor's memory/cache is seen by all CPUs. The use of mutex and other interlocks is not needed in this case. What exactly do they accomplish? They just create deadlocks and are not needed.

bool _flag;
#define memory_barrier __asm__ __volatile__ ("" ::: "memory") /* GCC */

void LoadingScreen::SetFlag(bool value)
{
    _flag = value;
    memory_barrier(); /* Ensure write happens immediately, even for in-lines */
}

bool LoadingScreen::GetFlag()
{
   bool value = _flag;
   memory_barrier(); /* Ensure read happens immediately, even for in-lines */
   return value;
}

Mutexes are only needed when multiple values are being set at the same time. You may also change the bool type to sig_atomic_t or LLVM atomics. However, this is rather pedantic as bool will work on most every practical CPU architecture. Cocoa's concurrency pages also have some information on alternative API's to do the same thing. I believe gcc's in-line assembler is the same syntax as used with Apple's compilers; but that could be wrong.

There are some limitations to the API. The instance GetFlag() returns, something can call SetFlag(). GetFlag() return value is then stale. If you have multiple writers, then you can easily miss one SetFlag(). This maybe important if the higher level logic is prone to ABA problems. However, all of these issue exist with/without mutexs. The memory barrier only solves the issue that a compiler/CPU will not cache the SetFlag() for a prolonged time and it will re-read the value in GetFlag(). Declaring volatile bool flag will generally result in the same behavior, but with extra side-effects and does not solve multi-CPU issues.

std::atomic<bool>As per stefan and atomic_set(&accessing_flag, true); will generally do the same thing as describe above in their implementations. You may wish to use them if they are available on your platforms.

Upvotes: 2

user1192878
user1192878

Reputation: 734

The code looks right if System::Mutex is correctly implemented. Something to be mentioned:

  1. As others pointed out, RAII is better than macro.

  2. It might be better to define accessingFlag and _flag as volatile.

  3. I think the temp solution you got is not correct if you compile with optimization.

    bool LoadingScreen::GetFlag()
{
  accessingFlag = true;  // might be reordered or deleted
  bool value = _flag;  // might be optimized away
  accessingFlag = false;    // might be reordered before value set
  return value;   // might be optimized to directly returen _flag or register
}
    In above code, optimizer could do nasty things. For example, there is nothing to prevent the compiler eliminate the first assignment to accessingFlag=true, or it could be reordered, cached. For example, for compiler point of view, if single-threaded, the first assignment to accessingFlag is useless because the value true is never used.

  4. Use mutex to protect a single bool variable is expensive since most of time spent on switching OS mode (from kernel to user back and forth). It might not be bad to use a spinlock (detail code depend on your target platform). It should be something like:

    spinlock_lock(&lock); _flag = value; spinlock_unlock(&lock);

  5. Also atomic variable is good here as well. It might look like:
atomic_set(&accessing_flag, true);

Upvotes: 1

DummyDDD
DummyDDD

Reputation: 52

The second block of code that you provided may modify the flag while it is being read, even in uni processor settings. The original code that you posted is correct, and cannot lead to deadlocks under two assumptions:

  1. The m_flags lock is correctly initialized, and not modified by any other code.
  2. The lock implementation is correct.

If you want a portable lock implementation, I would suggest using OpenMP: How to use lock in openMP?

From your description it seems like you want to busy wait for a thread to process some input. In this case, stefans solution (declare the flag std::atomic) is probably best. On semi-sane x86 systems, you could also declare the flag volatile int. Just don't do this for unaligned fields (packed structures).

You can avoid busy waiting with two locks. The first lock is unlocked by the slave when it finishes processing and locked by the main thread when waiting for the slave to finish. The second lock is unlocked by the main thread when providing input, and locked by the slave when waiting for input.

Upvotes: 0

ruben2020
ruben2020

Reputation: 1549

Here's a technique I've seen somewhere, but couldn't find the source anymore. If I find it, I will edit the answer. Basically, the writer will just write, but the reader will read the value of the set variable more than once, and only when all copies are consistent, it would use it. And I've changed the writer so that it will try to keep writing the value as long as it does not match the value it expects.

bool _flag;

void LoadingScreen::SetFlag(bool value)
{
    do
    {
       _flag = value;
    } while (_flag != value);
}

bool LoadingScreen::GetFlag()
{
    bool value;

    do
    {
        value = _flag;
    } while (value != _flag);

    return value;
}

Upvotes: -1

user2141076
user2141076

Reputation: 1

Have you considered using CRITICAL_SECTION? This is only available on Windows, so you lose some portability, but it is an effective user level mutex.

Upvotes: 0

Slava
Slava

Reputation: 44258

First of all you should use RAII for mutex lock/unlock. Second you either do not show some other code that uses _flag directly, or there is something wrong with mutex you are using (unlikely). What library provides System::Mutex?

Upvotes: 2

Related Questions