Microsoft's remark to ReaderWriterLockSlim.IsReadLockHeld/IsWriteLockHeld and its consequences

Question

To synchronize the access to my properties I use the ReaderWriterLockSlim class. I use the following code to access my properties in a thread-safe way.

public class SomeClass
{
    public readonly ReaderWriterLockSlim SyncObj = new ReaderWriterLockSlim();  
    public string AProperty
    {
        get
        {
            if (SyncObj.IsReadLockHeld)
                return ComplexGetterMethod();
            SyncObj.EnterReadLock();
            try
            {
                return ComplexGetterMethod();
            }
            finally
            {
                SyncObj.ExitReadLock();
            }
        }
        set
        {
            if (SyncObj.IsWriteLockHeld)
                ComplexSetterMethod(value);
            else
            {
                SyncObj.EnterWriteLock();
                ComplexSetterMethod(value);
                SyncObj.ExitWriteLock();
            }
        }
    }

    // more properties here ...

    private string ComplexGetterMethod()
    {
        // This method is not thread-safe and reads
        // multiple values, calculates stuff, ect. 
    }

    private void ComplexSetterMethod(string newValue)    
    {
        // This method is not thread-safe and reads
        // and writes multiple values.
    }
}

// =====================================

public static SomeClass AClass = new SomeClass();
public void SomeMultiThreadFunction()
{
    ...
    // access with locking from within the setter
    AClass.AProperty = "new value";
    ...
    // locking from outside of the class to increase performance
    AClass.SyncObj.EnterWriteLock();
    AClass.AProperty = "new value 2";
    AClass.AnotherProperty = "...";
    ...
    AClass.SyncObj.ExitWriteLock();
    ...
}

To avoid unnecessary locks whenever I get or set multiple properties a once I published the ReaderWriterLockSlim-Object and lock it from outside of the class every time I'm about to get or set a bunch of properties. To achieve this my getter and setter methods check if the lock has been acquired using the IsReadLockHeld property and the IsWriteLockHeld property of ReaderWriterLockSlim. This works fine and has increased the performance of my code.

So far so good but when I re-read the documentation about IsReadLockHeld and IsWriteLockHeld I noticed the remark form Microsoft:

This property is intended for use in asserts or for other debugging purposes. Do not use it to control the flow of program execution.

My question is: Is there a reason why I should not use IsReadLockHeld/IsWriteLockHeld for this purpose? Is there anything wrong with my code? Everything works as expected and much faster than using recursive locks (LockRecursionPolicy.SupportsRecursion).

To clarify this up: This is a minimal example. I don't want to know if the lock itself is necessary or can be removed or achieved in a different way. I just want to know why I should not use IsReadLockHeld/IsWriteLockHeld to control the flow of the programm as stated by the documentation.

Answer 1

After some further research I posted the same question on the German Support Forum of the Microsoft Developer Network and got into discussion with the very helpful moderator Marcel Roma. He was able to contact the programmer of the ReaderWriterLockSlim Joe Duffy who wrote this answer:

I'm afraid my answer may leave something to be desired.

The property works fine and as documented. The guidance really is just because conditional acquisition and release of locks tends to be buggy and error-prone in practice, particularly with exceptions thrown into the mix.

It's typically a good idea to structure your code so that you either use recursive acquires, or you don't, (and of course the latter is always easier to reason about); using properties like IsReadLockHeld lands you somewhere in the middle.

I was one of the primary designers of RWLS and I have to admit it has way too many bells and whistles. I don't necessarily regret adding IsReadLockHeld -- as it can come in handy for debugging and assertions -- however as soon as we added it, Pandora's box was opened, and we RWLS was instantly opened up to this kind of usage.

I'm not surprised that people want to use it as shown in the StackOverflow thread, and I'm sure there are some legitimate scenarios where it works better than the alternatives. I merely advise erring on the side of not using it.

To sum things up: You can use the IsReadLockHeld and the IsWriteLockHeld property to acquire a lock conditionally and everything will work fine, but it is bad programming style and one should avoid it. It is better to stick to recursive or non-recursive locks. To maintain a good coding style IsReadLockHeld and IsWriteLockHeld should only be used for debugging purposes.

I want to thank Marcel Roma and Joe Duffy again for their precious help.

Answer 2

Documentation is advising you the right thing.

Considere the following interleaved execution.

Thread1.AcqrireReadLock();
Thread1.ComplexGetterMethod();
Thread2.ReadIsReaderLockHeldProperty();
Thread1.ReleaseReadLock();
Thread2.ComplexGetterMethod(); // performing read without lock.

The other wrong thing with your code that I see is

SyncObj.EnterReadLock();
try
{
    return ComplexGetterMethod();
}
finally
{
    SyncObj.ExitReadLock();
}

is not the right way to do things. This is one right:

try
{
    SyncObj.EnterReadLock();

    return ComplexGetterMethod();
}
finally
{
    if (SyncObj.IsReadLockHeld)
        SyncObj.ExitReadLock();
}

And this shall be exact definition of your getter method.