mutexes : Is this thread-safe ?
Hi : I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads... ------------------------------------ class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { } private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------ I appreciate all the help.. How do I know to check for thread-safety in a code ? Are there any documents? I have checked online and have some links, but havent found documents which provide examples and explain.. such a document would be really useful. Thanks... Dhanvi
Dhanvi Kapila wrote:
Hi : I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads...
If you stop and think about it for a second, the principal reason mutexes, condvars etc were invented was to synchronise the operation of separate threads competing for access to a common memory space. So, it would be pointless having a mutex that was not thread safe. In the Unix world at least these synchronisation primitives are all defined in the _pthread_ library. Jim
Dhanvi Kapila wrote:
I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads... ------------------------------------ class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------
That depends on how you want to use this object. I assume that you want to share it between threads since there's no way to let two locks use the same mutex with this design. If this is the case, check out http://www.boost.org/doc/html/threads/concepts.html#threads.concepts.lock-co... and http://www.boost.org/doc/html/threads/rationale.html#threads.rationale.locks Let's assume that you have two threads. Thread A has already locked the shared BoostMutexLock when B tries to do the same. Now B waits until the mutex is unlocked. As soon as A calls unlock(), the lock object is destroyed and the mutex unlocked. Now it might happen that before the lock pointer is deleted, B is started and creates a new lock object, overwriting lk. The memory of the previous lock object is leaked. After that, A deletes lk, so the mutex is unlocked again while B is inside the protected section. Malte
Malte Clasen wrote:
Dhanvi Kapila wrote:
I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads... ------------------------------------ class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------
That depends on how you want to use this object. I assume that you want to share it between threads since there's no way to let two locks use the same mutex with this design.
Thats a very valid point. Two threads cannot share this lock object since each thread would have its own mutex object. This negates the whole point of achieving to do synchronization. But if I declare the mutex as static then all the threads would share just one mutex object and that too is something I dont want to do. I need to wrap boost mutexes in a interface so that I can abstract that at my application level. My application doesnt need to know how the locking is done. How would a shared_ptr help me in such a scenario ?? Thanks for your time and responses.. I really appreciate all the help. Dhanvi
Dhanvi Kapila wrote:
Hi : I have this piece of code for mutex operations. is this code thread-safe ?
Thread-safety aside, what is the use of this code? Why not just use a shared_ptr<mutex> and construct the lock objects on the stack, as they're supposed to be (and as is exception-safe, unlike your object)? Sebastian Redl
Dhanvi Kapila wrote:
Hi : I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads... ------------------------------------ class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------ I appreciate all the help.. How do I know to check for thread-safety in a code ? Are there any documents? I have checked online and have some links, but havent found documents which provide examples and explain.. such a document would be really useful.
That code goes against the whole point of synchronization primitives in boost.threads - it makes it easy to write code that leaves a lock unlocked, e.g. when an exception is thrown. Why not work with the library rather than against it? It will be threadsafe though, yes, since the scoped lock constructor will have to lock global_mutex before assigning to lk. If two threads on two CPUs run lock at the same time, they will create separate, independent scoped_lock objects, and one will get the lock and unlock it before the other one gets to assign lk, so there's no problem. It's inefficient though due to the memory allocation - why do you want it? Tom
Tom Widmer wrote:
Dhanvi Kapila wrote:
class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------
[snip]
It will be threadsafe though, yes, since the scoped lock constructor will have to lock global_mutex before assigning to lk. If two threads on two CPUs run lock at the same time, they will create separate, independent scoped_lock objects, and one will get the lock and unlock it before the other one gets to assign lk, so there's no problem. It's inefficient though due to the memory allocation - why do you want it?
I don't think it is threadsafe on every platform. It it my understanding that the following line: lk = new boost::mutex::scoped_lock(global_mutex); can be split by the compiler to be: lk = malloc(sizeof(boost::mutex::scoped_lock)); // alloc and assign // another thread could come in here, causing // leak of the first malloc and a later double // delete lk->boost::mutex::scoped_lock(global_mutex); // call constructor See, for example: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html for more info. KevinH -- Kevin Heifner heifner @ ociweb.com http://heifner.blogspot.com Object Computing, Inc. (OCI) www.ociweb.com
Kevin Heifner wrote:
Tom Widmer wrote:
Dhanvi Kapila wrote:
class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------
[snip]
It will be threadsafe though, yes, since the scoped lock constructor will have to lock global_mutex before assigning to lk. If two threads on two CPUs run lock at the same time, they will create separate, independent scoped_lock objects, and one will get the lock and unlock it before the other one gets to assign lk, so there's no problem. It's inefficient though due to the memory allocation - why do you want it?
I don't think it is threadsafe on every platform. It it my understanding that the following line: lk = new boost::mutex::scoped_lock(global_mutex); can be split by the compiler to be: lk = malloc(sizeof(boost::mutex::scoped_lock)); // alloc and assign // another thread could come in here, causing // leak of the first malloc and a later double // delete lk->boost::mutex::scoped_lock(global_mutex); // call constructor
That particular re-ordering is illegal. Remember, when a mutex is locked, no memory access or code that happens after the mutex is locked (in this case the assignment to lk) can be moved to before the mutex was locked (in other words, a mutex emits an appropriate memory barrier), so lk will not be assigned until after global_mutex is locked. For an example of why the reordering isn't allowed in the C++ abstract machine in the single threaded case, have a look at this: struct A; A* a = 0; struct A { A() { assert(a == 0); } }; int main() { a = new A; //must not assert delete a; } This is different from DCL, where one of the paths does *not* lock the mutex - in the OP's code, all paths lock the mutex before accessing shared data, so there's no problem. Tom
Tom Widmer wrote:
That particular re-ordering is illegal. Remember, when a mutex is locked, no memory access or code that happens after the mutex is locked (in this case the assignment to lk) can be moved to before the mutex was locked (in other words, a mutex emits an appropriate memory barrier), so lk will not be assigned until after global_mutex is locked.
[...]
This is different from DCL, where one of the paths does *not* lock the mutex - in the OP's code, all paths lock the mutex before accessing shared data, so there's no problem.
Right. The mutex is acquired in the constructor and therefore provides the memory barrier needed. It looks wrong to my eyes since I would never use raw new/delete in this context. Sorry for the noise. KevinH -- Kevin Heifner heifner @ ociweb.com http://heifner.blogspot.com Object Computing, Inc. (OCI) www.ociweb.com
Kevin Heifner wrote:
Right. The mutex is acquired in the constructor and therefore provides the memory barrier needed. It looks wrong to my eyes since I would never use raw new/delete in this context. Sorry for the noise.
So how you delete a lock ?? Is there any other safe way to delete a lock ? Thanks, Dhanvi
Dhanvi Kapila wrote:
Kevin Heifner wrote:
Right. The mutex is acquired in the constructor and therefore provides the memory barrier needed. It looks wrong to my eyes since I would never use raw new/delete in this context. Sorry for the noise.
So how you delete a lock ?? Is there any other safe way to delete a lock ?
I don't know what you are asking. Can you provide an example? Use the scoped lock concept. If you need to hide boost from the rest of the application then provide a wrapper around scoped lock. KevinH -- Kevin Heifner heifner @ ociweb.com http://heifner.blogspot.com Object Computing, Inc. (OCI) www.ociweb.com
Kevin Heifner wrote:
Dhanvi Kapila wrote:
So how you delete a lock ?? Is there any other safe way to delete a lock ?
I don't know what you are asking. Can you provide an example? Use the scoped lock concept. If you need to hide boost from the rest of the application then provide a wrapper around scoped lock I need to hide boost locks from the rest of my code. I have a factory class that creates locks ( we have ACE and Boost locks implemented). to create the locks, in boost, we use the new keyword and to unlock ( release the lock ) we delete the locks. The mutex object using which the locks are created are shared among the threads .
// create lock //my_mutex is a mutex factory object with b_mutex as a public member of type Boost::mutex boost::mutex::scoped_lock* lk = new boost::mutex::scoped_lock( my_mutex->b_mutex ); // deletion of lock delete lk; is this a correct way to do this ?? Thanks, Dhanvi..
Dhanvi Kapila wrote:
Kevin Heifner wrote:
Dhanvi Kapila wrote:
So how you delete a lock ?? Is there any other safe way to delete a lock ?
I don't know what you are asking. Can you provide an example? Use the scoped lock concept. If you need to hide boost from the rest of the application then provide a wrapper around scoped lock I need to hide boost locks from the rest of my code. I have a factory class that creates locks ( we have ACE and Boost locks implemented). to create the locks, in boost, we use the new keyword and to unlock ( release the lock ) we delete the locks. The mutex object using which the locks are created are shared among the threads .
// create lock //my_mutex is a mutex factory object with b_mutex as a public member of type Boost::mutex boost::mutex::scoped_lock* lk = new boost::mutex::scoped_lock( my_mutex->b_mutex );
// deletion of lock delete lk;
is this a correct way to do this ??
If you don't mind including ACE or Boost header files whichever you are using at the moment then something like this would work: // file MyMutex_ACE.h #include "ace/Synch_T.h" struct MyMutex { ACE_SYNCH_MUTEX mutex_; }; #define MY_SCOPED_LOCK(M) ACE_GUARD(ACE_SYNCH_MUTEX, ace_mon, M.mutex_) // file MyMutex_Boost.h #include <boost/thread.hpp> struct MyMutex { boost::mutex mutex_; }; #define MY_SCOPED_LOCK(M) boost::mutex::scoped_lock(M.mutex_) // file MyMutex.h #ifdef ACE #include "MyMutex_ACE.h" #else #include "MyMutex_Boost.h" #endif // application code #include "MyMutex.h" MyMutex mutex; MY_SCOPED_LOCK(mutex); If you want to not include ACE/Boost header files in your application code then something like the following would work: // MyMutex.h #incluce <boost/scoped_ptr.hpp> struct MyMutex { MyMutex(); struct MyMutexImpl; boost::scoped_ptr<MyMutexImpl> impl_; }; class MyScopedLock { public: MyScopedLock(MyMutex& m); private: struct MyScopedLockImpl; boost::scoped_ptr<MyScopedLockImpl> impl_; }; // MyMutex.cpp #include "MyMutex.h" #ifdef ACE #include "ace/Synch_T.h" struct MyMutex::MyMutexImpl { ACE_SYNCH_MUTEX mutex_; }; struct MyScopedLock::MyScopedLockImpl { MyScopedLockImpl(MyMutex& m) : lock_(m.impl_->mutex_) {} ACE_Guard<ACE_SYNCH_MUTEX> lock_; }; #else #include <boost/thread.hpp> struct MyMutex::MyMutexImpl { boost::mutex mutex_; }; struct MyScopedLock::MyScopedLockImpl { MyScopedLockImpl(MyMutex& m) : lock_(m.impl_->mutex_) {} boost::mutex::scoped_lock lock_; }; #endif MyMutex::MyMutex() : impl_(new MyMutexImpl) {} HTH, KevinH -- Kevin Heifner heifner @ ociweb.com http://heifner.blogspot.com Object Computing, Inc. (OCI) www.ociweb.com
Dhanvi Kapila wrote:
Hi : I have this piece of code for mutex operations. is this code thread-safe ? I plan to use this code in threads... ------------------------------------ class BoostMutexLock { public : void lock() { lk = new boost::mutex::scoped_lock(global_mutex); } void unlock() { delete lk; } BoostMutexLock() : lk( 0 ) { } ~BoostMutexLock() { }
private: boost::mutex global_mutex; boost::mutex::scoped_lock* lk; }; ------------------------------------------------------ I appreciate all the help.. How do I know to check for thread-safety in a code ? Are there any documents? I have checked online and have some links, but havent found documents which provide examples and explain.. such a document would be really useful.
Thanks... Dhanvi
This code is against to RAII (Resource Allocation Is Initialization) idiom. Would be better if you use it that way: try { boost::mutex::scoped_lock(global_mutex); ... <your code here> } catch { } if you don't need to use try-catch then just add a scope: { boost::mutex::scoped_lock(global_mutex); ... <your code here> } It's guarantied that mutex will be released when execution leaves the scope.
participants (13)
-
Dhanvi Kapila -
Emre Birol -
Jim Douglas -
Kevin Heifner -
Malte Clasen -
Sebastian Redl -
Tom Widmer