ReaderWriterLockSection: A bad idea?

Question

In writing some threaded code, I've been using the ReaderWriterLockSlim class to handle synchronized access to variables. Doing this, I noticed I was always writing try-finally blocks, the same for each method and property.

Seeing an opportunity to avoid repeating myself and encapsulate this behaviour I built a class, ReaderWriterLockSection, intended to be used as a thin wrapper to the lock which can be used with the C# using block syntax.

The class is mostly as follows:

public enum ReaderWriterLockType
{
   Read,
   UpgradeableRead,
   Write
}

public class ReaderWriterLockSection : IDisposeable
{
   public ReaderWriterLockSection(
       ReaderWriterLockSlim lock, 
       ReaderWriterLockType lockType)
   {
       // Enter lock.
   }

   public void UpgradeToWriteLock()
   {
       // Check lock can be upgraded.
       // Enter write lock.
   }

   public void Dispose()
   {
       // Exit lock.
   }
}

I use the section as follows:

private ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();

public void Foo()
{
    using(new ReaderWriterLockSection(_lock, ReaderWriterLockType.Read)
    {
        // Do some reads.
    }
}

To me, this seems like a good idea, one that makes my code easier to read and seemingly more robust since I wont ever forget to release a lock.

Can anybody see an issue with this approach? Is there any reason this is a bad idea?

Answer 1

Well, it seems okay to me. Eric Lippert has previously written about the dangers of using Dispose for "non-resource" scenarios, but I think this would count as a resource.

It may make life tricky in upgrade scenarios, but you could always fall back to a more manual bit of code at that point.

Another alternative is to write a single lock acquire/use/release method and provide the action to take while holding the lock as a delegate.

Answer 2

I usually indulge into this kind of code-sugary confections!

Here's a variant that's a bit easier to read for the users, on top of your API

public static class ReaderWriterLockExt{
  public static IDisposable ForRead(ReaderWriterLockSlim rwLock){
    return new ReaderWriterLockSection(rwLock,ReaderWriterLockType.Read);
  }
  public static IDisposable ForWrite(ReaderWriterLockSlim rwLock){
    return new ReaderWriterLockSection(rwLock,ReaderWriterLockType.Write);
  }
  public static IDisposable ForUpgradeableRead(ReaderWriterLockSlim wrLock){
    return new ReaderWriterLockSection(rwLock,ReaderWriterLockType.UpgradeableRead);
  }
}

public static class Foo(){
  private static readonly ReaderWriterLockSlim l=new ReaderWriterLockSlim(); // our lock
  public static void Demo(){


    using(l.ForUpgradeableRead()){ // we might need to write..

      if(CacheExpires()){   // checks the scenario where we need to write

         using(l.ForWrite()){ // will request the write permission
           RefreshCache();
         } // relinquish the upgraded write
      }

      // back into read mode
      return CachedValue();
    } // release the read 
  }
}

I also recommend using a variant that takes an Action delegate that's invoked when the lock cannot be obtained for 10 seconds, which I'll leave as an exercise to the reader.

You might also want to check for a null RWL in the static extension methods, and make sure the lock exists when you dispose it.

Cheers, Florian

Answer 3

There is another consideration here, you are possibly solving a problem you should not solve. I can't see the rest of your code but I can guess from you seeing value in this pattern.

Your approach solves a problem only if the code that reads or writes the shared resource throws an exception. Implicit is that you don't handle the exception in the method that does the reading/writing. If you did, you could simply release the lock in the exception handling code. If you don't handle the exception at all, the thread will die from the unhandled exception and your program will terminate. No point in releasing a lock then.

So there's a catch clause somewhere lower in the call stack that catches the exception and handles it. This code has to restore the state of the program so that it can meaningful continue without generating bad data or otherwise die due to exceptions caused by altered state. That code has a difficult job to do. It needs to somehow guess how much data was read or written without having any context. Getting it wrong, or only partly right, is potentially very destabilizing to the entire program. After all, it was a shared resource, other threads are reading/writing from it.

If you know how to do this, then by all means use this pattern. You better test the heck out of though. If you're not sure then avoid wasting system resources on a problem you can't reliably fix.