Error with ReaderWriterLockSlim

Question

I got this exception
The read lock is being released without being held.
at System.Threading.ReaderWriterLockSlim.ExitReadLock() at .. GetBreed(String)

Below is the only place in code that accesses the lock. As you can see, there is no recursion. I'm having trouble understanding how this exception could occur.

static readonly Dictionary<string, BreedOfDog> Breeds 
     = new Dictionary<string,BreedOfDog>();

static BreedOfDog GetBreed(string name)
{
        try
        {
            rwLock.EnterReadLock();
            BreedOfDog bd;
            if (Breeds.TryGetValue(name, out bd))
            {
                return bd;
            }
        }
        finally
        {
            rwLock.ExitReadLock();
        }

        try
        {
            rwLock.EnterWriteLock();
            BreedOfDog bd;
            //make sure it hasn't been added in the interim
            if (Breeds.TryGetValue(t, out bd)
            {
                return bd;
            }
            bd = new BreedOfDog(name); //expensive to fetch all the data needed to run  the constructor, hence the caching

            Breeds[name] = bd;
            return bd;
        }
        finally
        {
            rwLock.ExitWriteLock();
        }
}

Can you produce a short but complete program which demonstrates the problem? — Jon Skeet, Apr 24 '12 at 17:48
You shouldn't be acquiring the lock, letting go, then acquiring it. You should keep the lock from beginning to end and use an upgradable read lock, then upgrade if you need to write. — vcsjones, Apr 24 '12 at 17:48
@vcsjones I disagree - in most cases release and re-acquire (maybe double-check) is more effective — Marc Gravell, Apr 24 '12 at 17:56
@vcsjones only one thread can hold an upgradable read lock at a time — dan, Apr 24 '12 at 18:30
@dan indeed; it makes me wonder why they bother having it... you are *essentially* making it a mutex, so in that scenario it is not *very* different to just taking the write lock (at least, if we assume that most threads are using the same code, so would be wanting upgradeable too) — Marc Gravell, Apr 25 '12 at 09:04

score 3 · Accepted Answer · answered Apr 24 '12 at 18:00

I'm guessing you have something re-entrant, and it is throwing an exception when obtaining the lock. There is a catch-22 whether you "take the lock", "try" vs "try", "take the lock", but the "take the lock", "try" has fewer failure cases (the "aborted between take and try" is so vanishingly unlikely you don't need to stress).

Move the "take the lock" outside the "try", and see what the actual exception is.

The problem is most likely that you are failing to take the lock (probably re-entrancy), then trying to unlock something you didn't take. This could mean that the exception surfaces in the orginal code that took the lock, due to trying to release twice when only taken once.

Note: Monitor has new overloads with "ref bool" parameters to help with this scenario - but not the other lock types.

Brian Gideon · Answer 2 · 2012-04-24T18:55:14.773

Use LockRecursionPolicy.SupportsRecursion when instantiating the RWLS. If the error goes away then you actually do have some type of recursion involved. Perhaps it is in code that you did not post?

And if you are really concerned about getting maximum concurrency out of this (as I suspect you are since you are using a RWLS) then you could use the double-checked locking pattern. Notice how your original code already has that feel to it? So why beat around bush? Just do it.

In the following code notice how I always treat the Breeds reference as immutable and then inside the lock I recheck, copy, change, and swap out the reference.

static volatile Dictionary<string, BreedOfDog> Breeds = new Dictionary<string,BreedOfDog>();
static readonly object LockObject = new object();

static BreedOfDog GetBreed(string name)
{
  BreedOfDog bd;
  if (!Breeds.TryGetValue(name, out bd))
  {
    lock (LockObject)
    {
      if (!Breeds.TryGetValue(name, out bd))
      {
        bd = new BreedOfDog(name);
        var copy = new Dictionary<string, BreedOfDog>(Breeds);
        copy[name] = bd;
        Breeds = copy;
      }
    }
  }
  return bd;
}

@BitKFu: It wasn't that great. I removed my RWLS version because it wasn't concurrent at all. `EnterUpgradeableReadLock` would basically serialize the `GetBreed` method. — Brian Gideon, Apr 24 '12 at 19:01

Error with ReaderWriterLockSlim

2 Answers2