CODEWITHSUNDEEP
c++multithreading
Arsenii Fomin
Arsenii Fomin

Reputation: 3356

Is it safe lock/scope guard implementation?

For now I have this implementation of thread-safe field getting from a class:

int A::GetValue() const
{
    _mutex.Lock();
    int temp = _value;
    _mutex.Unlock();
    return temp;
}

I want to replace it with lock/scope guard implementation like this (LockGuard class has only constructor/destructor and Mutex * _mutex field):

LockGuard::LockGuard(Mutex & mutex) : _mutex(&mutex)
{
    _mutex->Lock();
}

LockGuard::~LockGuard()
{
    _mutex->Unlock();
}

And refactored code:

int A::GetValue() const
{
    LockGuard lockGuard(_mutex);
    return _value;
}

I suddendly realized that I don't sure if this a safe implementation. Is it guaranteed that firstly the copy of _value will be passed out of function and only with this copy already existed the _mutex will be unlocked? Please don't provide C++11 alternative implementation examples - I have embedded system and unfortunatelly can't use them.

Upvotes: 1

Views: 628

Answers (3)

Rakib
Rakib

Reputation: 7625

Your implementation of LockGuard is ok, but usage is wrong. With the statement 

LockGuard(_mutex);

You are creating a temporary LockGuard which will be destroyed just after the statement is finished.

Your usage should be

LockGuard guard(_mutex);
return _value;

LockGuard guard(_mutex); will create a LockGuard object, which will lock the mutex in constructor. When the method returns, the destructor will be called, unlocking the mutex. And you no longer have to use a temporary to return.

Upvotes: 4

jsantander
jsantander

Reputation: 5102

Perhaps this program my help you visualize what happens:

#include <iostream>

// Replaces the int so that we know when things happen
struct Int {
   Int() {
       std::cout << "Int::Int()" << std::endl;
   }
   ~Int() {
       std::cout << "Int::~Int()" << std::endl;
   }
   Int(const Int &x) {
       std::cout << "Int::Int(const Int&)" << std::endl;
   }
};

struct LockGuard {
   LockGuard() {
     std::cout << "Locking" << std::endl;
   }
   ~LockGuard() {
     std::cout << "Unlocking" << std::endl;
   }    
};

struct A {    
  Int getValue() const {
     LockGuard lockGuard;
     return _value;
  }    
  Int _value;    
};


int main() {
   A a;
   std::cout << "about to call" << std::endl;
   Int x=a.getValue();
   std::cout << "done calling" << std::endl;
}

That results in 

Int::Int()
about to call
Locking
Int::Int(const Int&)
Unlocking
done calling
Int::~Int()
Int::~Int()

One final note is that you might need to make _mutex mutable in your attribute declaration as locking and unlocking are typically non-const operation that would not be allowed in a const method otherwise.

Upvotes: 4

Jeremy Friesner
Jeremy Friesner

Reputation: 73379

This line is a problem:

LockGuard(_mutex);

It should be:

LockGuard foo(_mutex);

Otherwise it will go out of scope and unlock your mutex before the rest of the method executes. (you can verify the problematic behavior by temporarily inserting printf's or similar in the LockGuard constructor and destructor as well as in your calling method).

Other than that your locking pattern is good.

Upvotes: 5

Related Questions