3
\$\begingroup\$

I have implemented a simple read/write lock without using a critical section. I have tested my code by running 50 threads reading and writing the same data.

.h file

#pragma once
#include "stdafx.h"
class CReadWriteLock
{
public:
    CReadWriteLock(void);
    virtual ~CReadWriteLock(void);
    void LockReader();
    void UnlockReader();
    void LockWriter();
    void UnlockWriter();
private:
    CReadWriteLock(const CReadWriteLock &cReadWriteLock);
    const CReadWriteLock& operator=(const CReadWriteLock &cReadWriteLock);
    void IncrementReaderCount();
    void DecrementReaderCount();
    HANDLE m_hWriteMutex;
    HANDLE m_hNoReadersEvent;
    int m_nReaderCount;
};

.cpp file

#include "StdAfx.h"
#include "ReadWriteLock.h"


CReadWriteLock::CReadWriteLock(void)
 : m_nReaderCount(0), m_hWriteMutex(NULL), m_hNoReadersEvent(NULL)
{
    m_hWriteMutex    = CreateMutex(NULL, FALSE, NULL);
    m_hNoReadersEvent = CreateEvent(NULL, TRUE, TRUE, NULL);
}

CReadWriteLock::CReadWriteLock(const CReadWriteLock &cReadWriteLock)
{
}

const CReadWriteLock& CReadWriteLock::operator=(const CReadWriteLock &cReadWriteLock)
{
    return *this;
}


CReadWriteLock::~CReadWriteLock(void)
{
    LockWriter(); // This will wait for finishing of all read/write operation.
    UnlockWriter();
    CloseHandle(m_hWriteMutex);
    CloseHandle(m_hNoReadersEvent);
}


void CReadWriteLock::LockReader()
{
    WaitForSingleObject(m_hWriteMutex, INFINITE); // This will acquire write mutex
    IncrementReaderCount();
    ReleaseMutex(m_hWriteMutex); // Release write mutex
}


void CReadWriteLock::UnlockReader()
{
    // Decrement number of readers.
    WaitForSingleObject(m_hWriteMutex, INFINITE); // This will acquire write mutex
    DecrementReaderCount();
    ReleaseMutex(m_hWriteMutex); // Release write mutex

}


void CReadWriteLock::LockWriter()
{
    HANDLE handles[] = {m_hWriteMutex, m_hNoReadersEvent}; 
    WaitForMultipleObjects(2, handles, TRUE, INFINITE);
}


void CReadWriteLock::UnlockWriter()
{
    // Release Write mutex.
    ReleaseMutex(m_hWriteMutex);
}


void CReadWriteLock::IncrementReaderCount()
{

    m_nReaderCount++;
    // Reset the no readers event.
    ResetEvent(m_hNoReadersEvent);

}


void CReadWriteLock::DecrementReaderCount()
{

    m_nReaderCount--;
    // Are all readers out?
    if(m_nReaderCount <= 0)
    {
        // Set the no readers event.
        SetEvent(m_hNoReadersEvent);
    }

}

I want to know if I have overlooked something.

// ReadWriteLockTest.cpp :
//

#include "stdafx.h"
#include "ReadWriteLock.h"

const int THREADCOUNT = 50;
const int LOOP = 1000;

DWORD WINAPI TestFunction( LPDWORD );

CReadWriteLock glock;
int gData = 0;
int* gpData = 0;

int _tmain(int argc, _TCHAR* argv[])
{
    HANDLE aThread[THREADCOUNT];
    DWORD ThreadID;
    DWORD i;


    // Create worker threads
    gpData = new int(10); // Init
    for( i=0; i < THREADCOUNT; i++ )
    {
        aThread[i] = CreateThread( 
                     NULL,       // default security attributes
                     0,          // default stack size
                     (LPTHREAD_START_ROUTINE) TestFunction, 
                     NULL,       // no thread function arguments
                     0,          // default creation flags
                     0); 

        if( aThread[i] == NULL )
        {
            printf("CreateThread error: %d\n", GetLastError());
            delete gpData;
            return 1;
        }
    }

    // Wait for all threads to terminate

    WaitForMultipleObjects(THREADCOUNT, aThread, TRUE, INFINITE);

    // Close thread and mutex handles

    for( i=0; i < THREADCOUNT; i++ )
        CloseHandle(aThread[i]);

    delete gpData;
    return 0;
}

DWORD WINAPI TestFunction( LPDWORD lpParam )
{ 

    for(int h = 0 ; h < LOOP; ++h)
    {

        if(h % 2 != 0)
        {
            // Reader
            glock.LockReader();
            printf_s("H= %d Reader Data= %d, DataPtr= %d\n", h, gData, *gpData);
            if(h%5 == 0)
                Sleep(10);
            glock.UnlockReader();
        }
        else
        {
            // Writer
            glock.LockWriter();
            printf_s("H= %d Writer\n", h);
            if(h%3 == 0)
                Sleep(10);
            gData = h;
            delete gpData;
            gpData = new int(h*2); 
            glock.UnlockWriter();
        }


    }
    return TRUE; 
}

This code breaks if I use more than 70 threads. Is my test case correct?

\$\endgroup\$
  • \$\begingroup\$ Technically you still use a critical section by locking on the write mutex always. \$\endgroup\$ – ratchet freak Jun 4 '15 at 8:41
  • \$\begingroup\$ One would argue that 70 threads falls under Correctness in unanticipated cases. I would definitely never create that many threads. \$\endgroup\$ – Martin York Jun 4 '15 at 18:11
  • \$\begingroup\$ I too doubt that. I tried the same example with just a mutex and that too failed. \$\endgroup\$ – Shubham Jun 5 '15 at 9:29
  • 1
    \$\begingroup\$ I am reopening this question following the domain-expert comment by @LokiAstari - this code works in reasonable situations. \$\endgroup\$ – rolfl Jun 5 '15 at 11:21
  • \$\begingroup\$ @rolfl It makes sense. It falls in the tricky corners of "does not work". We could even say that nobody would have voted to close the question if OP hadn't realized that it didn't work with that many threads. \$\endgroup\$ – Morwenn Jun 5 '15 at 11:33
1
\$\begingroup\$

Note: global instantiation of objects like glock may give undefined results.

--

Outside of a controlled environment nothing guarantees that there will be no race between destruction and a "legitimate" lock. When you destroy a mutex (I'm not speaking about a specific implementation) you have to know nobody is using it. So the code below does not solve anything.

LockWriter(); // This will wait for finishing of all read/write operation.
UnlockWriter();

--

You should set m_nReaderCount as volatile as it can be modified outside of the running thread. If a compiler gets smart (i.e.: global optimization), the value may not be read/written the way you expect.

--

About testing for m_nReaderCount <= 0. In DecrementReaderCount you should assert that m_nReaderCount > 0 and only test for equality with 0. So if a bug creeps in you'll know immediately.

\$\endgroup\$
  • \$\begingroup\$ I used global instance cause I am required to guard a global instance object. As you mentioned I have made m_nReaderCount volatile also, added assert. other than this do you see any reason why this is causing heap a corruption when no of thread exceed 60-70? \$\endgroup\$ – Shubham Jun 4 '15 at 11:56
  • \$\begingroup\$ The missing volatile could break the mutex. \$\endgroup\$ – Hurricane Jun 4 '15 at 12:36
  • \$\begingroup\$ Since the data of your test is also shared, it also should be volatile (being in a loop it is even more likely to break). You may want to have another, simpler, test with writers that are incrementing a volatile global int a specific amount of time each (N millions). At the end you should see if the sum is writercount * N. \$\endgroup\$ – Hurricane Jun 4 '15 at 12:42
  • \$\begingroup\$ About the global object you may be interested in using a singleton mechanism or simply instantiate a global pointer (the risk is mostly allocation/construction before "main" is called). Note that this is most likely not your current issue. \$\endgroup\$ – Hurricane Jun 4 '15 at 12:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.