views:

786

answers:

2

I'm very new to multi-threading and for some reason this class is giving me more trouble than it should.

I am setting up a dictionary in the ASP.net cache - It will be frequently queried for individual objects, enumerated occasionally, and written extremely infrequently. I'll note that the dictionary data is almost never changed, I'm planning on letting it expire daily with a callback to rebuild from the database when it leaves the cache.

I believe that the enumeration and access by keys are safe so long as the dictionary isn't being written. I'm thinking a ReaderWriterLockSlim based wrapper class is the way to go but I'm fuzzy on a few points.

If I use Lock I believe that I can either lock on a token or the actual object I'm protecting. I don't see how to do something similar using the ReaderWriter Lock. Am I correct in thinking that multiple instances of my wrapper will not lock properly as the ReaderWriterLocks are out of each other's scope?

What is the best practice for writing a wrapper like this? Building it as a static almost seems redundant as the primary object is being maintained by the cache. Singleton's seem to be frowned upon, and I'm concerned about the above mentioned scoping issues for individual instances.

I've seen a few implementations of similar wrappers around but I haven't been able to answer these questions. I just want to make sure that I have a firm grasp on what I'm doing rather than cutting & pasting my way through. Thank you very much for your help!


**Edit: Hopefully this is a clearer summary of what I'm trying to find out- **

1. Am I correct in thinking that the lock does not affect the underlying data and is scoped like any other variable?

As an example lets say i have the following -

MyWrapperClass 
{
    ReaderWriterLockSlim lck = new ReaderWriterLockSlim();
    Do stuff with this lock on the underlying cached dictionary object...
}

MyWrapperClass wrapA = new MyWrapperClass();
MyWrapperClass wrapB = new MyWrapperClass();

Am I right in thinking that the wrapA lock and wrapB lock won't interact, And that if wrapA & wrapB both attempt operations it will be unsafe?

2. If this is the case what is the best practice way to "share" the lock data?

This is an Asp.net app - there will be multiple pages that need to access the data which is why i'm doing this in the first place. What is the best practice for ensuring that the various wrappers are using the same lock? Should my wrapper be a static or singleton that all threads are using, if not what is the more elegant alternative?

A: 

The standard way to lock is: object lck = new object(); ... lock(lck) { ... } in this instance the object lck represents the lock.

ReadWriterLockSlim isn't much different, its just in this case the actual ReadWriterLockSlim class represents the actual lock, so everywhere you would have used lck you now use your ReadWriterLockSlim.

ReadWriterLockSlim lck = new ReadWriterLockSlim();

...

lck.EnterReadLock();
try
{
    ...
}
finally
{
    lck.ExitReadLock();
}
KeeperOfTheSoul
That is how I have the lock set up now, but if my wrapper object has multiple instances I'm concerned the lck objects will not be in the same scope and therefore will not interact as intended. I could of course make the lck a static variable, or make my entire wrapper class static but i want to see if it is necessary, and correct to do so. Thanks!
apocalypse9
Neither is a good plan there, ensure you only have one such wrapper class. In fact stop thinking of it as a wrapper class and think of it as the actual class, the fact it uses a dictionary being an implementation detail of that class. At that point you shouldn't reveal the dictionary outside said class.
KeeperOfTheSoul
+2  A: 

You have multiple dictionary objects in the Cache, and you want each one locked independently. The "best" way is to just use a simple class that does it for you.

public class ReadWriteDictionary<K,V>
{
    private readonly Dictionary<K,V> dict = new Dictionary<K,V>();
    private readonly ReaderWriterLockSlim rwLock = new ReaderWriterLockSlim();

    public V Get(K key)
    {
        return ReadLock(() => dict[key]);
    }

    public void Set(K key, V value)
    {
        WriteLock(() => dict.Add(key, value));
    }

    public IEnumerable<KeyValuePair<K, V>> GetPairs()
    {
        return ReadLock(() => dict.ToList());
    }

    private V2 ReadLock<V2>(Func<V2> func)
    {
        rwLock.EnterReadLock();
        try
        {
            return func();
        }
        finally
        {
            rwLock.ExitReadLock();
        }
    }

    private void WriteLock(Action action)
    {
        rwLock.EnterWriteLock();
        try
        {
            action();
        }
        finally
        {
            rwLock.ExitWriteLock();
        }
    }
}

Cache["somekey"] = new ReadWriteDictionary<string,int>();

There is also a more complete example on the help page of ReaderWriterLockSlim on MSDN. It wouldn't be hard to make it generic.

edit To answer your new questions -

1.) You are correct wrapA and wrapB will not interact. They both have their own instance of ReaderWriterLockSlim.

2.) If you need a shared lock amongst all your wrapper classes, then it must be static.

Justin Rudd
I was just reworking the question as you posted. This actually completely sidesteps the issues I was concerned about. I was thinking that I'd cache the dictionary internally and the wrapper would be external, but your solution definitely seems more elegant. Thanks!
apocalypse9
Perfect- Thank you very much! Caching the entire object as opposed to just the dictionary seems like a much better approach. I really appreciate the clarification, I was afraid of moving forward without being sure of what I was doing and I couldn't seem to find a solid example that confirmed one way or the other. Sorry to change the question on you as you were posting! Again I appreciate all the help that made thing much clearer!
apocalypse9
WARNING!!! . Enumerating a dictionary is not thread safe even with reading it. There should be write lock with GetPairs() not read lock. So the above code might fail as it is now
Do you have a example that shows it will become corrupted?
Justin Rudd