-2

After going through this question with the same title and its answers, I thought to try something that should really work only using critical section and thus should be much faster that existing solutions (which use other kernel objects too like mutex or semaphore)

Here are my Read/Write lock/unlock functions:

#include <windows.h>

typedef struct _RW_LOCK 
{
    CRITICAL_SECTION readerCountLock;
    CRITICAL_SECTION writerLock;
    int readerCount;
} RW_LOCK, *PRW_LOCK;

void InitLock(PRW_LOCK rwlock)
{
    InitializeCriticalSection(&rwlock->readerCountLock);
    InitializeCriticalSection(&rwlock->writerLock);
}

void ReadLock(PRW_LOCK rwlock)
{
    EnterCriticalSection(&rwlock->readerCountLock); // In deadlock 1 thread waits here (see description below)
    if (++rwlock->readerCount == 1) 
    {
        EnterCriticalSection(&rwlock->writerLock); // In deadlock 1 thread waits here
    }
    LeaveCriticalSection(&rwlock->readerCountLock);
}

void ReadUnlock(PRW_LOCK rwlock)
{
    EnterCriticalSection(&rwlock->readerCountLock);
    if (--rwlock->readerCount == 0) 
    {
        LeaveCriticalSection(&rwlock->writerLock);
    }
    LeaveCriticalSection(&rwlock->readerCountLock);
}

int WriteLock(PRW_LOCK rwlock)
{
    EnterCriticalSection(&rwlock->writerLock); // In deadlock 3 threads wait here
}

void WriteUnlock(PRW_LOCK rwlock)
{
    LeaveCriticalSection(&rwlock->writerLock);
}

And here is a thread function. After calling InitLock (&g_rwLock); from main I created FIVE threads to try these locks.

void thread_function()
{
    static int value = 0;
    RW_LOCK g_rwLock;
    
    while(1)
    {
        ReadLock(&g_rwlLock);
        BOOL bIsValueOdd = value % 2;
        ReadUnlock(&g_rwlLock);

        WriteLock(&g_rwlLock);
        value ++;
        WriteUnlock(&g_rwlLock);
    }
}

Ideally this code should keep running forever without any trouble. But to my disappointment, it doesn't run always. Some times it lands up in deadlock. I compiled this and ran it on Windows XP. To create threads using threadpool, I am using third party library. Hence cannot give here all that code which involves lots of initializing routines and other stuff.

But to cut the story short, I like to know if anyone by looking at the code above can point out what is wrong with this approach?

I've commented in the code above where each thread (out of FIVE threads) keeps waiting when deadlock happens. (I found it out by attaching debugger to the deadlocked process)

Any inputs/suggestions would be really great as I've stuck over this for quite some time now (In the greed of making my code run faster than ever).

Community
  • 1
  • 1
Atul
  • 3,778
  • 5
  • 47
  • 87
  • Note that this is a reader-preferenced lock -- if your reader count never reaches 0, a write will wait forever. If this isn't desirable, you may want to think about a ticketing system as in the [Mellor-Crummey-Scott](http://www.cs.rochester.edu/u/scott/papers/1991_PPoPP_read_write.pdf) "fair" lock. – Cory Nelson Dec 01 '14 at 20:10
  • As usual: _RW_LOCK is a name reserved for the compiler (leading underscore followed by a capital letter) –  Dec 01 '14 at 20:11
  • 1
    Please post the *actual* code that demonstrates the issue. The code here has clearly lost something in translation and is wasting helpful people's time. – Cory Nelson Dec 01 '14 at 21:03
  • `InitLock` has been called from `main`. I realized there are mistakes happened while posting. I am really under stress :( Let me try to prepare code that won't use any third party library, and post it here. – Atul Dec 02 '14 at 03:23
  • 1
    Now that the mystery is solved (ala Steve's answer) note that if you're after performance, using interlocked operations to increment and decrement `readerCount` would be substantially faster than a critical section. – Harry Johnston Dec 02 '14 at 03:47

2 Answers2

4

Spotted two things so far:

  • You initialize the critical sections in every thread, which is not allowed (behavior is undefined)
  • You can't leave a critical section in a different thread from the one that entered it ("If a thread calls LeaveCriticalSection when it does not have ownership of the specified critical section object, an error occurs that may cause another thread using EnterCriticalSection to wait indefinitely.")

The latter fits the deadlock you see.

Once you have multiple readers concurrently, you don't control which order they call ReadUnlock, so you can't ensure that the first thread in, which is the only one allowed to call LeaveCriticalSection, is the last one out.

Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
  • Note each thread has a different critical section. Obviously this is not what he intended, but it means `LeaveCriticalSection` on a separate thread doesn't account for a deadlock. – Cory Nelson Dec 01 '14 at 20:23
  • @Cory: oh yeah, well spotted. In that case nothing should block ever, and there's a race condition on `value`. UB per the standard, but won't have these symptoms on Windows. – Steve Jessop Dec 01 '14 at 20:50
  • Actually, come to think of it, what's `Value`? Not the same as `value`. So I don't know now what we can trust in the code shown, and what we can't. If there's actually only one critical section in the real code, maybe it's only inited once, but my second point could still apply. – Steve Jessop Dec 01 '14 at 21:03
  • `InitLock` has been called actually from `main`. But I suspect your second point is applicable here. After a thread calls `ReadLock` and `ReadUnlock` if another thread goes ahead of it calling `WriteLock` and `WriteUnlock`, it essentially calls `LeaveCriticalSection` on the critical section object which it never have entered into. – Atul Dec 02 '14 at 03:29
0

This way it cannot run correctly.

  • lets 1 thread enter ReadLock(), let it pass ++ instruction but pause it before entering writer CS
  • another thread enters WriteLock() and successfuly enters writerCS

so now we have readers count = 1 and running writer at the same time. note that reader is deadlocked on EnterCriticalSection(&rwlock->writerLock)

Anonymous
  • 2,122
  • 19
  • 26