views:

1041

answers:

4

I have the following piece of code:

private Dictionary<object, object> items = new Dictionary<object, object>;
public IEnumerable<object> Keys
{
    get
    {
        foreach (object key in items.Keys)
        {
            yield return key;
        }
    }
}

Is this thread-safe? If not do I have to put a lock around the loop or the yield return?

Here is what I mean:

Thread1 accesses the Keys property while Thread2 adds an item to the underlying dictionary. Is Thread1 affected by the add of Thread2?

+12  A: 

What exactly do you mean by thread-safe?

You certainly shouldn't change the dictionary while you're iterating over it, whether in the same thread or not.

If the dictionary is being accessed in multiple threads in general, the caller should take out a lock (the same one covering all accesses) so that they can lock for the duration of iterating over the result.

EDIT: To respond to your edit, no it in no way corresponds to the lock code. There is no lock automatically taken out by an iterator block - and how would it know about syncRoot anyway?

Moreover, just locking the return of the IEnumerable<TKey> doesn't make it thread-safe either - because the lock only affects the period of time when it's returning the sequence, not the period during which it's being iterated over.

Jon Skeet
Yes, I meant Dictionary instead of List. The first edit was wrong (sorry about that) and I removed it. I added the expected behavior to the question.
Albic
+6  A: 

Check out this post on what happens behind the scenes with the yield keyword:

Behind the scenes of the C# yield keyword

In short - the compiler takes your yield keyword and generates an entire class in the IL to support the functionality. You can check out the page after the jump and check out the code that gets generated...and that code looks like it tracks thread id to keep things safe.

Justin Niessner
+1 for the really helpful link.
Albic
+2  A: 

I believe it is, but I cannot find a reference that confirms it. Each time any thread calls foreach on an iterator, a new thread local* instance of the underlying IEnumerator should get created, so there should not be any "shared" memory state that two threads can conflict over...

  • Thread Local - In the sense that it's reference variable is scoped to a method stack frame on that thread
Charles Bretana
Yeah but what if call GetEnumerator and then share the IEnumerator ?He should clarify what he is doing with his threads.
Guillaume
+1  A: 

OK, I did some testing and got an interesting result.

It seems that it is more an issue of the enumerator of the underlying collection than the yield keyword. The enumerator (actually its MoveNext method) throws (if implemented correctly) an InvalidOperationException because the enumeration has changed. According to the MSDN documentation of the MoveNext method this is the expected behavior.

Because enumerating through a collection is usually not thread-safe a yield return is not either.

Albic