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.

The ReaderWriteLockSlim doesn't protect anything, .NET already promises that object reference updates are atomic. The only mild side effect it will have is that the setter updates will be visible in other threads. Much cheaper to just use MemoryBarrier(). No thread-safety guarantees whatsoever, you are certainly better off removing SyncObj completely since it only gives a false sense of safety. — Hans Passant, Jun 11 '13 at 16:13
@EricLippert: The docs for `IsRead/WriteLockHeld` suggest that they return `true` when the *current thread* holds the lock. If `IsRead/WriteLockHeld` returns true, how can the lock state have changed on that thread without an explicit call to `ExitRead/WriteLock`? — Iridium, Jun 11 '13 at 17:27
@Iridium: I withdraw my comment; I'm now actually not sure why the documentation makes that caveat. That said: best practice is to do what the documentation says. — Eric Lippert, Jun 11 '13 at 17:51
@HansPassant: You are right for this example but imagine a property where the getter and/or the setter function is more complex than `return _AProperty;` or `_AProperty = value;`. The getter may create a string depending on other values of the class, the setter could change multiple values or create new objects. These complex tasks cannot be done in a simple atomic statement so locking is required. — Shinja, Jun 11 '13 at 20:18
You were asking "is there anything wrong with **this** code". Yes, there is. Trying to guess that there is anything wrong with code we cannot see is very unproductive. — Hans Passant, Jun 11 '13 at 20:23
`SyncObj` should be readonly so that it cannot ever be swapped out. This is even more true because you've made it public. — Tergiver, Jun 11 '13 at 21:32
I made some changes to the question to clarify things up. The code above is just a abstraction to show what I'm doing. I don't want know if I need the lock or not, or how to implement it. All I want to know is why shouldn't one use `IsReadLockHeld`/`IsWriteLockHeld` to control the flow of the program. Sorry if that didn't came out clearly in the first place. — Shinja, Jun 12 '13 at 06:59
I know this is a very old thread, but I'm using the ReaderWriterLockSlim in an almost identical scenario. I noticed that after releasing all my locks, the debugger would indicate that the ReadLock was still held. After hours of trying to isolate the problem, I found that the code could pass an Assert(SyncObj.IsReadLockHeld), but the debugger was still adamant that a ReadLock was held. — Quark Soup, Dec 01 '18 at 19:23
It's my conclusion that the debugger isn't able to accurately report on the state of a ReaderWriterLockSlim. Oddly, I've see a scenario where the Watch window is showing that the IsReaderLockHeld is false while the pop-up of view of the same object shows that it is held. I'm guessing that the Debugger isn't always running on the same thread as the code. Anyway, I'm going to continue on with this design pattern, but place liberal Assert statements until I'm positive it's just a debugging artifact. — Quark Soup, Dec 01 '18 at 19:40

score 14 · Accepted Answer · answered Jun 25 '13 at 11:27

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.

score -2 · Answer 2 · answered Jun 16 '13 at 13:09

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.

That execution wouldn't have occurred that way, because `IsReadLockHeld` returns `true` only when the **current thread** holds the lock. — svick, Jun 16 '13 at 15:41

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

2 Answers2