views:

176

answers:

4

I have a class that maintains a private Dictionary instance that caches some data.

The class writes to the dictionary from multiple threads using a ReaderWriterLockSlim.

I want to expose the dictionary's values outside the class.
What is a thread-safe way of doing that?

Right now, I have the following:

public ReadOnlyCollection<MyClass> Values() {
    using (sync.ReadLock())
        return new ReadOnlyCollection<MyClass>(cache.Values.ToArray()); 
}

Is there a way to do this without copying the collection many times?

I'm using .Net 3.5 (not 4.0)

A: 

Review next possibility, just exposes ICollection interface, so in Values() you can return your own implementation. This implementation will use only reference on Dictioanry.Values and always use ReadLock for access items.

Dewfy
This won't work because the lock is private. (Consumers of the class cannot enter the lock)
SLaks
+1  A: 

EDIT: I personally believe the below code is technically answering your question correctly (as in, it provides a way to enumerate over the values in a collection without creating a copy). Some developers far more reputable than I strongly advise against this approach, for reasons they have explained in their edits/comments. In short: This is apparently a bad idea. Therefore I'm leaving the answer but suggesting you not use it.


Unless I'm missing something, I believe you could expose your values as an IEnumerable<MyClass> without needing to copy values by using the yield keyword:

public IEnumerable<MyClass> Values {
    get {
        using (sync.ReadLock()) {
            foreach (MyClass value in cache.Values)
                yield return value;
        }
    }
}

Be aware, however (and I'm guessing you already knew this), that this approach provides lazy evaluation, which means that the Values property as implemented above can not be treated as providing a snapshot.

In other words... well, take a look at this code (I am of course guessing as to some of the details of this class of yours):

var d = new ThreadSafeDictionary<string, string>();

// d is empty right now
IEnumerable<string> values = d.Values;

d.Add("someKey", "someValue");

// if values were a snapshot, this would output nothing...
// but in FACT, since it is lazily evaluated, it will now have
// what is CURRENTLY in d.Values ("someValue")
foreach (string s in values) {
    Console.WriteLine(s);
}

So if it's a requirement that this Values property be equivalent to a snapshot of what is in cache at the time the property is accessed, then you're going to have to make a copy.

(begin 280Z28): The following is an example of how someone unfamiliar with the "C# way of doing things" could lock the code:

IEnumerator enumerator = obj.Values.GetEnumerator();
MyClass first = null;
if (enumerator.MoveNext())
    first = enumerator.Current;

(end 280Z28)

Dan Tao
Your `yield break` is useless. Also, this can be a property instead of a method. (Since it doesn't do an expensive copy)
SLaks
@SLaks: Good points. I updated my suggestion accordingly.
Dan Tao
@Dan: I would *strongly* recommend avoiding this. The potential performance improvement carries a great risk of deadlocking the dictionary in read-only mode. When it comes to thread-safe code, the thread safety *always* trumps performance.
280Z28
@Dan: I added an example of "poorly written but otherwise innocent code" that would lock the dictionary.
280Z28
**This is a very, very bad idea.** Arbitrarily much time can pass between the yield and the next iteration, time during which other code could be triggering events that cause other threads to try to access the collection. This code is handing users a time bomb that causes deadlocks when it goes off. **Always avoid taking out a lock when you cannot control (1) how much time is spent in the lock, and (2) what code can run during the lock**.
Eric Lippert
@280Z28: That's a very helpful observation, and one I definitely hadn't considered, in all honesty. Personally, while I agree in general that "thread safety ... trumps performance," I also think there's a distinction to be made between scenarios where a dead-lock is *possible*--but avoidable, if the development team is properly informed--and where a dead-lock is inevitable given the design of a particular class/method/etc. In a performance-critical scenario, I might still opt for the above approach if it provided a performance improvement (I don't even know; I haven't profiled it).
Dan Tao
I'm aware that this code has the potential to be incredibly evil, but I'm using it anyway. This isn't a public library - I am the only user of the code, and I already understand these issues.
SLaks
+2  A: 

If you want a snapshot of the current state of the dictionary, there's really nothing else you can do with this collection type. This is the same technique used by the ConcurrentDictionary<TKey, TValue>.Values property.

If you don't mind throwing an InvalidOperationException if the collection is modified while you are enumerating it, you could just return cache.Values since it's readonly (and thus can't corrupt the dictionary data).

280Z28
+5  A: 

I want to expose the dictionary's values outside the class. What is a thread-safe way of doing that?

You have three choices.

1) Make a copy of the data, hand out the copy. Pros: no worries about thread safe access to the data. Cons: Client gets a copy of out-of-date data, not fresh up-to-date data. Also, copying is expensive.

2) Hand out an object that locks the underlying collection when it is read from. You'll have to write your own read-only collection that has a reference to the lock of the "parent" collection. Design both objects carefully so that deadlocks are impossible. Pros: "just works" from the client's perspective; they get up-to-date data without having to worry about locking. Cons: More work for you.

3) Punt the problem to the client. Expose the lock, and make it a requirement that clients lock all views on the data themselves before using it. Pros: No work for you. Cons: Way more work for the client, work they might not be willing or able to do. Risk of deadlocks, etc, now become the client's problem, not your problem.

Eric Lippert