Почему в некоторых случаях не работает цикл foreach?

Ваша проблема в том, что конструктор List<T>, который создает новый список из IEnumerable (который вы называете), не является потокобезопасным в отношении своего аргумента. Что происходит, пока это:

 new List<string>(Foo.Requests)

выполняется, другой поток изменяет Foo.Requests. Вам придется заблокировать его на время этого звонка.

[РЕДАКТИРОВАТЬ]

Как указал Эрик, другая проблема List<T> не гарантирует безопасность чтения читателями, пока другой поток ее изменяет. Т.е. одновременные читатели - это нормально, но одновременные читатели и писатели - нет. И хотя вы блокируете записи друг против друга, вы не блокируете чтение и запись.

Ваша схема блокировки нарушена. Вам нужно заблокировать Foo.Requests() на всю продолжительность цикла, а не только при удалении элемента. В противном случае элемент может стать недействительным в середине вашей операции «обработки данных», и перечисление может измениться между переходами от элемента к элементу. И это предполагает, что вам не нужно вставлять коллекцию в течение этого интервала. Если это так, вам действительно нужно повторно фактор, чтобы использовать правильную очередь производителя / потребителя.

Увидев ваше исключение; мне кажется, что Foo.Requests изменяется, пока создается неглубокая копия. Измените его на что-то вроде этого:

List<string> requests;

lock (Foo.Requests)
{
    requests = new List<string>(Foo.Requests);
}

foreach (string data in requests)
{
    // Process the data.

    lock (Foo.Requests)
    {
        Foo.Requests.Remove(data);
    }
}

Не вопрос, но ...

При этом я несколько сомневаюсь, что вышесказанное - то, чего вы хотите. Если во время обработки поступают новые запросы, они не будут обработаны, когда ваш цикл foreach завершится. Поскольку мне было скучно, вот кое-что, что, я думаю, вы пытаетесь достичь:

class RequestProcessingThread
{
    // Used to signal this thread when there is new work to be done
    private AutoResetEvent _processingNeeded = new AutoResetEvent(true);

    // Used for request to terminate processing
    private ManualResetEvent _stopProcessing = new ManualResetEvent(false);

    // Signalled when thread has stopped processing
    private AutoResetEvent _processingStopped = new AutoResetEvent(false);

    /// <summary>
    /// Called to start processing
    /// </summary>
    public void Start()
    {
        _stopProcessing.Reset();

        Thread thread = new Thread(ProcessRequests);
        thread.Start();
    }

    /// <summary>
    /// Called to request a graceful shutdown of the processing thread
    /// </summary>
    public void Stop()
    {
        _stopProcessing.Set();

        // Optionally wait for thread to terminate here
        _processingStopped.WaitOne();
    }

    /// <summary>
    /// This method does the actual work
    /// </summary>
    private void ProcessRequests()
    {
        WaitHandle[] waitHandles = new WaitHandle[] { _processingNeeded, _stopProcessing };

        Foo.RequestAdded += OnRequestAdded;

        while (true)
        {
            while (Foo.Requests.Count > 0)
            {
                string request;
                lock (Foo.Requests)
                {
                    request = Foo.Requests.Peek();
                }

                // Process request
                Debug.WriteLine(request);

                lock (Foo.Requests)
                {
                    Foo.Requests.Dequeue();
                }
            }

            if (WaitHandle.WaitAny(waitHandles) == 1)
            {
                // _stopProcessing was signalled, exit the loop
                break;
            }
        }

        Foo.RequestAdded -= ProcessRequests;

        _processingStopped.Set();
    }

    /// <summary>
    /// This method will be called when a new requests gets added to the queue
    /// </summary>
    private void OnRequestAdded()
    {
        _processingNeeded.Set();
    }
}


static class Foo
{
    public delegate void RequestAddedHandler();
    public static event RequestAddedHandler RequestAdded;

    static Foo()
    {
        Requests = new Queue<string>();
    }

    public static Queue<string> Requests
    {
        get;
        private set;
    }

    public static void AddRequest(string request)
    {
        lock (Requests)
        {
            Requests.Enqueue(request);
        }

        if (RequestAdded != null)
        {
            RequestAdded();
        }
    }
}

С этим все еще есть несколько проблем, которые я оставлю читателю:

• Проверка _stopProcessing, вероятно, должна выполняться после каждой обработки запроса
• Подход Peek () / Dequeue () не будет работать, если у вас есть несколько потоков, выполняющих обработку
• Недостаточная инкапсуляция: Foo.Requests доступен, но Foo.AddRequest необходимо использовать для добавления любых запросов, если вы хотите, чтобы они обрабатывались.
• В случае нескольких потоков обработки: необходимо обрабатывать пустую очередь внутри цикла, поскольку нет блокировки вокруг проверки Count> 0.

Честно говоря, я бы предложил провести рефакторинг. Вы удаляете элементы из объекта, одновременно выполняя итерацию над ним. Ваш цикл может выйти до того, как вы обработаете все элементы.

Три вещи:
- Я бы не блокировал их внутри оператора for (each), но за его пределами.
- Я бы не блокировал фактическую коллекцию, а локальный статический объект
- Вы не может изменить список / коллекцию, которую вы перечисляете

Для получения дополнительной информации проверьте:
http://msdn.microsoft.com/en-us/library/c5kehkcz(VS.80).aspx.

lock (lockObject) {
   foreach (string data in new List<string>(Foo.Requests))
        Foo.Requests.Remove(data);
}

Проблема в выражении

new List<string>(Foo.Requests)

внутри вашего foreach, потому что он не заблокирован. Я предполагаю, что пока .NET копирует вашу коллекцию запросов в новый список, список изменяется другим потоком.

foreach (string data in new List<string>(Foo.Requests))
{
    // Process the data.
    lock (Foo.Requests)
    {
        Foo.Requests.Remove(data);
    }
}

Предположим, у вас есть два потока, выполняющих этот код.

в System.Collections.Generic.List1 [System.String] .. ctor

• Thread1 начинает обработку списка.
• Thread2 вызывает конструктор List, который подсчитывает количество создаваемого массива.
• Thread1 изменяет количество элементов в списке.
• В Thread2 неверное количество элементов.

Ваша схема блокировки неправильная. Это даже неправильно в примере цикла for.

Вам необходимо блокировать каждый раз, когда вы получаете доступ к общему ресурсу - даже для его чтения или копирования. Это не означает, что вам нужно блокировать всю операцию. Это означает, что каждый, кто использует этот общий ресурс, должен участвовать в схеме блокировки.

Также рассмотрите защитное копирование:

List<string> todos = null;
List<string> empty = new List<string>();
lock(Foo.Requests)
{
  todos = Foo.Requests;
  Foo.Requests = empty;
}

//now process local list todos

Даже в этом случае все те, кто использует Foo.Requests, должны участвовать в схеме блокировки.

Вы пытаетесь удалить объекты из списка, когда выполняете итерацию по списку. (Хорошо, технически вы этого не делаете, но это цель, которую вы пытаетесь достичь).

Вот как вы это делаете правильно: во время итерации создайте еще один список записей, которые вы хотите удалить. Просто создайте другой (временный) список, поместите все записи, которые вы хотите удалить из исходного списка, во временный список.

List entries_to_remove = new List(...);

foreach( entry in original_list ) {
   if( entry.someCondition() == true ) { 
      entries_to_remove.add( entry );
   }
}

// Then when done iterating do: 
original_list.removeAll( entries_to_remove );

Использование метода removeAll класса List.

Я знаю, что это не то, о чем вы просили, но просто ради моего рассудка, следующее представляет собой намерение вашего кода:

private object _locker = new object();

// ...

lock (_locker) {
    Foo.Requests.Clear();
}