Silverlight ReaderWriterLock Implementation Good/Bad?

Question

Hi, I have an adopted implementation of a simple (no upgrades or timeouts) ReaderWriterLock for Silverlight, I was wondering anyone with the right expertise can validate if it is good or bad by design. To me it looks pretty alright, it works as advertised, but I have limited experience with multi-threading code as such.

public sealed class ReaderWriterLock
{
    private readonly object syncRoot = new object();    // Internal lock.
    private int i = 0;                                  // 0 or greater means readers can pass; -1 is active writer.
    private int readWaiters = 0;                        // Readers waiting for writer to exit.
    private int writeWaiters = 0;                       // Writers waiting for writer lock.
    private ConditionVariable conditionVar;             // Condition variable.

    public ReaderWriterLock()
    {
        conditionVar = new ConditionVariable(syncRoot);
    }

    /// <summary>
    /// Gets a value indicating if a reader lock is held.
    /// </summary>
    public bool IsReaderLockHeld
    {
        get
        {
            lock ( syncRoot )
            {
                if ( i > 0 )
                    return true;
                return false;
            }
        }
    }

    /// <summary>
    /// Gets a value indicating if the writer lock is held.
    /// </summary>
    public bool IsWriterLockHeld
    {
        get
        {
            lock ( syncRoot )
            {
                if ( i < 0 )
                    return true;
                return false;
            }
        }
    }

    /// <summary>
    /// Aquires the writer lock.
    /// </summary>
    public void AcquireWriterLock()
    {
        lock ( syncRoot )
        {
            writeWaiters++;
            while ( i != 0 )
                conditionVar.Wait();      // Wait until existing writer frees the lock.
            writeWaiters--;
            i = -1;             // Thread has writer lock.
        }
    }

    /// <summary>
    /// Aquires a reader lock.
    /// </summary>
    public void AcquireReaderLock()
    {
        lock ( syncRoot )
        {
            readWaiters++;
            // Defer to a writer (one time only) if one is waiting to prevent writer starvation.
            if ( writeWaiters > 0 )
            {
                conditionVar.Pulse();
                Monitor.Wait(syncRoot);
            }
            while ( i < 0 )
                Monitor.Wait(syncRoot);
            readWaiters--;
            i++;
        }
    }

    /// <summary>
    /// Releases the writer lock.
    /// </summary>
    public void ReleaseWriterLock()
    {
        bool doPulse = false;
        lock ( syncRoot )
        {
            i = 0;
            // Decide if we pulse a writer or readers.
            if ( readWaiters > 0 )
            {
                Monitor.PulseAll(syncRoot); // If multiple readers waiting, pulse them all.
            }
            else
            {
                doPulse = true;
            }
        }
        if ( doPulse )
            conditionVar.Pulse();                     // Pulse one writer if one waiting.
    }

    /// <summary>
    /// Releases a reader lock.
    /// </summary>
    public void ReleaseReaderLock()
    {
        bool doPulse = false;
        lock ( syncRoot )
        {
            i--;
            if ( i == 0 )
                doPulse = true;
        }
        if ( doPulse )
            conditionVar.Pulse();                     // Pulse one writer if one waiting.
    }

    /// <summary>
    /// Condition Variable (CV) class.
    /// </summary>
    public class ConditionVariable
    {
        private readonly object syncLock = new object(); // Internal lock.
        private readonly object m;                       // The lock associated with this CV.

        public ConditionVariable(object m)
        {
            lock (syncLock)
            {
                this.m = m;
            }
        }

        public void Wait()
        {
            bool enter = false;
            try
            {
                lock (syncLock)
                {
                    Monitor.Exit(m);
                    enter = true;
                    Monitor.Wait(syncLock);
                }
            }
            finally
            {
                if (enter)
                    Monitor.Enter(m);
            }
        }

        public void Pulse()
        {
            lock (syncLock)
            {
                Monitor.Pulse(syncLock);
            }
        }

        public void PulseAll()
        {
            lock (syncLock)
            {
                Monitor.PulseAll(syncLock);
            }
        }

    }

}

If it is good, it might be helpful to others too as Silverlight currently lacks a reader-writer type of lock. Thanks.

Answer 1

Both of your IsReadorLockHeld and IsWriterLockHeld methods are flawed at a conceptual level. While it is possible to determine that at a given point in time a particular lock is or is not held, there is absolutely nothing you can safely do without this information unless you continue to hold the lock (not the case in your code).

These methods would be more accurately named WasReadLockHeldInThePast and WasWriterLockHeldInThePast. Once you rename the methods to a more accurate representation of what they do, it becomes clearer that they are not very useful.

Answer 2

This class seems simpler to me, and provides the same functionality. It may be slightly less performant, since it always PulsesAll(), but the logic is much simpler to understand, and I doubt the performance hit is that great.

public sealed class ReaderWriterLock()
{
    private readonly object internalLock = new object();
    private int activeReaders = 0;
    private bool activeWriter = false;

    public void AcquireReaderLock()
    {
        lock (internalLock)
        {
            while (activeWriter)
                Monitor.Wait(internalLock);
            ++activeReaders;
        }
    }

    public void ReleaseReaderLock()
    {
        lock (internalLock)
        {
            // if activeReaders <= 0 do some error handling
            --activeReaders;
            Monitor.PulseAll(internalLock);
        }
    }

    public void AcquireWriterLock()
    {
        lock (internalLock)
        {
            // first wait for any writers to clear 
            // This assumes writers have a higher priority than readers
            // as it will force the readers to wait until all writers are done.
            // you can change the conditionals in here to change that behavior.
            while (activeWriter)
                Monitor.Wait(internalLock);

            // There are no more writers, set this to true to block further readers from acquiring the lock
            activeWriter = true;

            // Now wait till all readers have completed.
            while (activeReaders > 0)
                Monitor.Wait(internalLock);

            // The writer now has the lock
        }
    }

    public void ReleaseWriterLock()
    {
        lock (internalLock)
        {
            // if activeWriter != true handle the error
            activeWriter = false;
            Monitor.PulseAll(internalLock);
        }
    }
}

Answer 3

I go in depth on explaining Vance Morrison's ReaderWriterLock (which became ReaderWriterLockSlim in .NET 3.5) on my blog (down to the x86 level). This might be helpful in your design, especially understanding how things really work.