Shouldn't this fail without the use of locking? Simple producer consumer

I have a queue, a list with producer threads and a list with consumer threads.

My code looks like this

    public class Runner
{
    List<Thread> Producers;
    List<Thread> Consumers;
    Queue<int> queue;
    Random random;

    public Runner()
    {
        Producers = new List<Thread>();
        Consumers = new List<Thread>();

        for (int i = 0; i < 2; i++)
        {
            Thread thread = new Thread(Produce);
            Producers.Add(thread);
        }

        for (int i = 0; i < 2; i++)
        {
            Thread thread = new Thread(Consume);
       开发者_StackOverflow中文版     Consumers.Add(thread);
        }

        queue = new Queue<int>();
        random = new Random();

        Producers.ForEach(( thread ) => { thread.Start(); });
        Consumers.ForEach(( thread ) => { thread.Start(); });
    }

    protected void Produce()
    {
        while (true)
        {
                int number = random.Next(0, 99);
                queue.Enqueue(number);
                Console.WriteLine(Thread.CurrentThread.ManagedThreadId + " Produce: " + number);
        }
    }

    protected void Consume()
    {
        while (true)
        {
                if (queue.Any())
                {
                    int number = queue.Dequeue();
                    Console.WriteLine(Thread.CurrentThread.ManagedThreadId + " Consume: " + number);
                }
                else
                {
                    Console.WriteLine("No items to consume");
                }
        }
    }
}

Shouldn't this fail miserable cause of the missing use of the lock keyword? It failed once because it tried to dequeue when the queue was empty, using the lock keyword will fix that right?

If the lock keyword is not needed for the above code, when is it needed then?

Thank you in advance! =)

Locking is to done to eliminate aberrant behavior of an application, most specifically in multithreading. The most common goal is the elimination of a "race condition" which causes non-deterministic program behavior.

This is the behavior you saw. In one run you get an error for the queue having no items, in another run you have no issues. This is a race condition. Proper usage of locking will eliminate this scenario.

Using Queue without locks is not thread safe indeed. But better than using locks you may try ConcurrentQueue. Google for "C# ConcurrentQueue" and you will find quite a lot of examples, e.g. this one compares the use and performance of Queue with a lock and ConcurrentQueue.

To clarify the existing answers, if you have a multithreading problem (such as a race condition) then it isn't guaranteed to always fail - it may fail, in a very unpredictable manner.

The reason is that two (or more) threads that are accessing a resource may try to access it at different times - precisely when each of them tries to access it will depend on many factors (how fast your CPU is, how many processor cores it has available, what other programs are running at the time, whether you are running a release or debug build, or running under a debugger, etc). You could run it many times without the failure showing up, and then have it suddenly and "inexplicably" fail - this can make these errors extremely hard to track down because they don't often show up while you're writing the faulty code, but more often when you are writing a different unrelated piece of code.

If you are going to use multithreading it is vital that you read up on the subject and gain an understanding of what can go wrong, when, and how to handle it properly - bad use of locking can be just as dangerous (if not more so) than not using locks at all (locking can cause deadlocks where your program simply "locks up"). This are aof programming must be approached carefully!

Yes this code will fail. The queue needs to support multi-threading. Use a ConcurrentQueue. See http://msdn.microsoft.com/en-us/library/dd267265.aspx

By running your code I received InvalidOperationException - "Collection was modified after the enumerator was instantiated." It means that you modify data while using several threads.

You can use the lock every time you Enqueue or Dequeue - because you modify the queue from several threads. A far better option is to use ConcurentQueues as it is thread safe and lock-free concurrent collection. It also provides better performance.

Yep, you would definitely to synchronize access to the Queue to make it thread-safe. But, you have another problem. There is no mechanism which keeps the consumers from spinning wildly around the loop. Synchronizing access to the Queue or using ConcurrentQueue will not fix that problem.

The simplest way to implement the producer-consumer pattern is to use a blocking queue. Fortunately, .NET 4.0 provides the BlockingCollection which is, despite the name, an implementation of a blocking queue.

public class Runner
{
    private BlockingCollection<int> queue = new BlockingCollection<int>();
    private Random random = new Random();

    public Runner()
    {
        for (int i = 0; i < 2; i++)
        {
            var thread = new Thread(Produce);
            thread.Start();
        }

        for (int i = 0; i < 2; i++)
        {
            var thread = new Thread(Consume);
            thread.Start();
        }
    }

    protected void Produce()
    {
        while (true)
        {
            int number = random.Next(0, 99);
            queue.Add(number);
            Console.WriteLine(Thread.CurrentThread.ManagedThreadId + " Produce: " + number);
        }
    }

    protected void Consume()
    {
        while (true)
        {
            int number = queue.Take();
            Console.WriteLine(Thread.CurrentThread.ManagedThreadId + " Consume: " + number);
        }
    }
}