HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Mutex implementation

Submitted by: @import:stackexchange-codereview··
0
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

#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:

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"
#endif


Don'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
    #endif


You 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.hpp

Code 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"
#endif
void 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.