31

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?

Albic
  • 3,559
  • 4
  • 22
  • 25
  • Your second exemple will lock, return an enumerable then unlock. And you will iterate AFTER the unlock. – Guillaume Sep 04 '09 at 14:14
  • Oh, you're right. I just noticed it is a bad example. I will edit the question. – Albic Sep 04 '09 at 14:22
  • Just to shed a little light: Thread-safety through locking is expensive, so it does not make sense to automatically lock on every action unless you explicitly ask for it. – Cecil Has a Name Sep 04 '09 at 16:47
  • Oops: I meant that the `yield`-statement shouldn't implicitly lock since you lose performance when thread-safety might not be needed in all cases. – Cecil Has a Name Sep 04 '09 at 16:48

6 Answers6

20

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
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 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 Sep 04 '09 at 14:31
18

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
  • 242,243
  • 40
  • 408
  • 536
13

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
  • 3,559
  • 4
  • 22
  • 25
4

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
  • 143,358
  • 22
  • 150
  • 216
  • Yeah but what if call GetEnumerator and then share the IEnumerator ? He should clarify what he is doing with his threads. – Guillaume Sep 04 '09 at 14:19
4

I believe yield implementation is thread-safe. Indeed, you can run that simple program at home and you will notice that the state of the listInt() method is correctly saved and restored for each thread without edge effect from other threads.

public class Test
{
    public void Display(int index)
    {
        foreach (int i in listInt())
        {
            Console.WriteLine("Thread {0} says: {1}", index, i);
            Thread.Sleep(1);
        }

    }

    public IEnumerable<int> listInt()
    {
        for (int i = 0; i < 5; i++)
        {
            yield return i;
        }
    }
}

class MainApp
{
    static void Main()
    {
        Test test = new Test();
        for (int i = 0; i < 4; i++)
        {
            int x = i;
            Thread t = new Thread(p => { test.Display(x); });
            t.Start();
        }

        // Wait for user
        Console.ReadKey();
    }
}
Chris
  • 41
  • 1
2
class Program
{
    static SomeCollection _sc = new SomeCollection();

    static void Main(string[] args)
    {
        // Create one thread that adds entries and
        // one thread that reads them
        Thread t1 = new Thread(AddEntries);
        Thread t2 = new Thread(EnumEntries);

        t2.Start(_sc);
        t1.Start(_sc);
    }

    static void AddEntries(object state)
    {
        SomeCollection sc = (SomeCollection)state;

        for (int x = 0; x < 20; x++)
        {
            Trace.WriteLine("adding");
            sc.Add(x);
            Trace.WriteLine("added");
            Thread.Sleep(x * 3);
        }
    }

    static void EnumEntries(object state)
    {
        SomeCollection sc = (SomeCollection)state;
        for (int x = 0; x < 10; x++)
        {
            Trace.WriteLine("Loop" + x);
            foreach (int item in sc.AllValues)
            {
                Trace.Write(item + " ");
            }
            Thread.Sleep(30);
            Trace.WriteLine("");
        }
    }
}

class SomeCollection
{
    private List<int> _collection = new List<int>();
    private object _sync = new object();

    public void Add(int i)
    {
        lock(_sync)
        {
            _collection.Add(i);
        }
    }


    public IEnumerable<int> AllValues
    {
        get
        {
            lock (_sync)
            {
                foreach (int i in _collection)
                {
                    yield return i;
                }
            }
        }
    }
}
sjp
  • 1,247
  • 1
  • 8
  • 2
  • Here is a simple example showing that yield is NOT thread safe by itself. As written it is thread safe but if you comment out the lock(_sync) in AllValues you should be able to verify that it is not thread safe by running it a few times. If you get an InvalidOperationException it proves that it is NOT thread safe. – sjp Oct 24 '12 at 17:38