patterncppMinor
Mutex implementation
Viewed 0 times
implementationmutexstackoverflow
Problem
Overarching Problem
I am creating a threadsafe queue that will act as a shared buffer between two threads for a game I am developing. I need it to be threadsafe so that one thread can throw messages into the queue, and second thread can take them out for processing in real time. I have decided in order to really learn about concurrency and threading I would like to create my own threading library with the required synchronization structures(starting off with a mutex that is implemented as a spinlock). For now I am using the Poco Threading library in order to test and develop my mutex. I will move forward to creating a Windows/Linux threading environment after I have my basic synchronization structures.
Proposed Solution (mutex/spinlock)
Note:
Sender and Receiver do not actually do any sending or receiving, or any queue work, they are just simply operating on numbers in tight for-loops to simulate work. I did not want to add extra variables to the test before I verified that my mutex implementation was working as expected.
Mutex.hpp
Mutex.cpp
Testing code
Note: For easy reading I simply included one thread
I am creating a threadsafe queue that will act as a shared buffer between two threads for a game I am developing. I need it to be threadsafe so that one thread can throw messages into the queue, and second thread can take them out for processing in real time. I have decided in order to really learn about concurrency and threading I would like to create my own threading library with the required synchronization structures(starting off with a mutex that is implemented as a spinlock). For now I am using the Poco Threading library in order to test and develop my mutex. I will move forward to creating a Windows/Linux threading environment after I have my basic synchronization structures.
Proposed Solution (mutex/spinlock)
Note:
Sender and Receiver do not actually do any sending or receiving, or any queue work, they are just simply operating on numbers in tight for-loops to simulate work. I did not want to add extra variables to the test before I verified that my mutex implementation was working as expected.
Mutex.hpp
#ifdef WIN32
#include
#define TARG_PLATFORM_WINDOWS
#else
#include
#define TARG_PLATFORM_LINUX
#endif
//Constant values
const int MUTEX_LOCKED = 1;
const int MUTEX_UNLOCKED = 0;
class Mutex
{
private:
volatile unsigned long long interlock;
public:
Mutex();
~Mutex();
void lock();
void unlock();
};Mutex.cpp
#include "Mutex.hpp"
Mutex::Mutex()
{
interlock = 0;
}
Mutex::~Mutex()
{
}
void Mutex::lock()
{
#ifdef TARG_PLATFORM_WINDOWS
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1);
#endif
#ifdef TARG_PLATFORM_LINUX
while(this->interlock == 1 || __sync_lock_test_and_set(&this->interlock, 1) == 1);
#endif
}
void Mutex::unlock()
{
this->interlock = 0;
#ifdef TARG_PLATFORM_WINDOWS
#endif
#ifdef TARG_PLATFORM_LINUX
#endif
}Testing code
Note: For easy reading I simply included one thread
Solution
@William Morris mentioned the spinning on a lock issue (busy wait). This is a real problem that you should address.
Empty block at the end of while is non obvious to read.
You should comment on it so that people don't think it is a mistake:
Your lock is not exception safe:
Use RAII to guarantee that locks are correctly locked/unlocked in the presence of exceptions:
There are more than two types of platform.
You should make sure these are covered. If the code does not fall into a particular type then you should generate an error: http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system
Don't put conditional code into functions:
You should define macros based on your conditional tags in the header file. Use the macros in your code. This should make it clear what you are trying to achieve.
Then in the header file:
You better have a good reason fir using a pointer:
Final Note:
Use an already implemented locking mechanism rather than rolling your own.
Empty block at the end of while is non obvious to read.
You should comment on it so that people don't think it is a mistake:
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1);
// I would do:
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1)
{ /* Do Nothing. While we wait to get the lock */
}Your lock is not exception safe:
Use RAII to guarantee that locks are correctly locked/unlocked in the presence of exceptions:
class Mutex
{
private:
volatile unsigned long long interlock;
public:
Mutex();
~Mutex();
private:
// These should be private to prevent locks being obtained
// in a way that makes them exception unsafe.
void lock();
void unlock();
// Need a friend that can lock and unlock the Mutex safely and correctly.
friend class Locker;
};
class Locker
{
Mutex& m;
public:
Locker(Mutex& m): m(m) {m.lock();}
~Locker() {m.unlock();}
};
// Usage
int main()
{
Mutex m;
for(;;)
{
Locker lock(m); // Locks the mutex (and gurantees it will be released)
// Even if there is an exception
}
}There are more than two types of platform.
You should make sure these are covered. If the code does not fall into a particular type then you should generate an error: http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system
#if defined(WIN32)
#include
#define TARG_PLATFORM_WINDOWS
#elif defined(__unix)
#include
#define TARG_PLATFORM_LINUX
#else
// Fail if this is a system you have not compensated for:
#error "Unknown System Type"
#endifDon't put conditional code into functions:
void Mutex::unlock()
{
this->interlock = 0;
#ifdef TARG_PLATFORM_WINDOWS
#endif
#ifdef TARG_PLATFORM_LINUX
#endif
}You should define macros based on your conditional tags in the header file. Use the macros in your code. This should make it clear what you are trying to achieve.
void Mutex::unlock()
{
this->interlock = 0;
UNLOCK_MY_MUTEX(interlock);
}Then in the header file:
#ifdef TARG_PLATFORM_WINDOWS
#define UNLOCK_MY_MUTEX(x) WindowsUnlockCode(x)
#endif
#ifdef TARG_PLATFORM_LINUX
#define UNLOCK_MY_MUTEX(x) /* Nothing required */ do {} while(false)
// see http://codereview.stackexchange.com/a/1686/507
#endifYou better have a good reason fir using a pointer:
mutex->lock();Final Note:
Use an already implemented locking mechanism rather than rolling your own.
pthread_mutex // Valid on Windows and Unix (if you get the libraries).
boost::spinlock // http://www.boost.org/doc/libs/1_38_0/boost/detail/spinlock.hppCode Snippets
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1);
// I would do:
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1)
{ /* Do Nothing. While we wait to get the lock */
}class Mutex
{
private:
volatile unsigned long long interlock;
public:
Mutex();
~Mutex();
private:
// These should be private to prevent locks being obtained
// in a way that makes them exception unsafe.
void lock();
void unlock();
// Need a friend that can lock and unlock the Mutex safely and correctly.
friend class Locker;
};
class Locker
{
Mutex& m;
public:
Locker(Mutex& m): m(m) {m.lock();}
~Locker() {m.unlock();}
};
// Usage
int main()
{
Mutex m;
for(;;)
{
Locker lock(m); // Locks the mutex (and gurantees it will be released)
// Even if there is an exception
}
}#if defined(WIN32)
#include <Windows.h>
#define TARG_PLATFORM_WINDOWS
#elif defined(__unix)
#include <unistd.h>
#define TARG_PLATFORM_LINUX
#else
// Fail if this is a system you have not compensated for:
#error "Unknown System Type"
#endifvoid Mutex::unlock()
{
this->interlock = 0;
#ifdef TARG_PLATFORM_WINDOWS
#endif
#ifdef TARG_PLATFORM_LINUX
#endif
}void Mutex::unlock()
{
this->interlock = 0;
UNLOCK_MY_MUTEX(interlock);
}Context
StackExchange Code Review Q#28083, answer score: 8
Revisions (0)
No revisions yet.