¿Por qué un bucle foreach no funciona en ciertos casos?

votos
4

Estaba usando un bucle foreach para ir a través de una lista de datos para procesar (eliminar dichos datos una vez procesados, esto estaba dentro de un bloqueo). Este método causaba una excepción ArgumentException de vez en cuando.

Cogerlo habría sido costoso, así que traté de rastrear el problema, pero no pude resolverlo.

Desde entonces, cambié a un ciclo for y el problema parece haberse solucionado. ¿Alguien puede explicar lo que pasó? Incluso con el mensaje de excepción, no entiendo muy bien lo que sucedió detrás de escena.

¿Por qué el bucle for aparentemente está funcionando? ¿Configuré el bucle Foreach mal o qué?

Esto es más o menos cómo se configuraron mis loops:

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

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

y

for (int i = 0; i < Foo.Requests.Count; i++)
{
    string data = Foo.Requests[i];

    // Process the data.

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

EDITAR: El bucle for * está en una configuración de tiempo así:

while (running)
{
    // [...]
}

EDITAR: Agregó más información sobre la excepción según lo solicitado.

System.ArgumentException: Destination array was not long enough. Check destIndex and length, and the array's lower bounds
  at System.Array.Copy (System.Array sourceArray, Int32 sourceIndex, System.Array destinationArray, Int32 destinationIndex, Int32 length) [0x00000] 
  at System.Collections.Generic.List`1[System.String].CopyTo (System.String[] array, Int32 arrayIndex) [0x00000] 
  at System.Collections.Generic.List`1[System.String].AddCollection (ICollection`1 collection) [0x00000] 
  at System.Collections.Generic.List`1[System.String]..ctor (IEnumerable`1 collection) [0x00000]

EDITAR: El motivo del bloqueo es que hay otro hilo que agrega datos. Además, con el tiempo, más de un subproceso procesará los datos (por lo que si la configuración completa es incorrecta, avise).

EDITAR: Fue difícil elegir una buena respuesta.

Me pareció que el comentario de Eric Lippert lo merecía, pero en realidad no respondió (de todos modos votó por su comentario).

Pavel Minaev, Joel Coehoorn y Thorarin dieron todas las respuestas que me gustaron y volvieron a votar. Thorarin también demoró 20 minutos adicionales para escribir un código útil.

Yo, que podía aceptar los 3 y dividir la reputación, pero por desgracia.

Pavel Minaev es el próximo merecedor, así que se lleva el mérito.

Gracias por la ayuda buena gente. :)

Publicado el 22/07/2009 a las 19:44
fuente por usuario
En otros idiomas...                            


9 respuestas

votos
13

Su problema es que el constructor del List<T>que crea una nueva lista IEnumerable(que es lo que llama) no es seguro para subprocesos con respecto a su argumento. Lo que sucede es que mientras esto:

 new List<string>(Foo.Requests)

se está ejecutando, otro hilo cambia Foo.Requests. Deberá bloquearlo durante toda la llamada.

[EDITAR]

Como lo señaló Eric, List<T>tampoco se garantiza otro problema seguro para que los lectores lo lean mientras otro hilo lo está cambiando. Es decir, los lectores concurrentes están bien, pero el lector y el escritor concurrentes no lo son. Y mientras bloquea sus escrituras entre sí, no bloquea sus lecturas contra sus escrituras.

Respondida el 22/07/2009 a las 19:58
fuente por usuario

votos
5

Después de ver su excepción; me parece que Foo.Requests se está cambiando mientras se está construyendo la copia superficial. Cambiarlo a algo como esto:

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);
    }
}

No es la pregunta, pero ...

Dicho esto, dudo un poco de que lo anterior sea lo que quieras tampoco. Si ingresan nuevas solicitudes durante el procesamiento, no se procesarán cuando termine su ciclo foreach. Como estaba aburrido, aquí hay algo en la línea que creo que estás tratando de lograr:

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();
        }
    }
}

Todavía hay algunos problemas con esto, que dejaré al lector:

  • La comprobación de _stopProcessing probablemente debería realizarse después de cada vez que se procesa una solicitud
  • El enfoque Peek () / Dequeue () no funcionará si tiene varios hilos procesando
  • Insuficiente encapsulación: Foo.Requests es accesible, pero Foo.AddRequest debe usarse para agregar cualquier solicitud si desea que se procesen.
  • En el caso de múltiples subprocesos de procesamiento: es necesario que la cola esté vacía dentro del bucle, ya que no hay ningún bloqueo alrededor de la verificación Count> 0.
Respondida el 22/07/2009 a las 19:59
fuente por usuario

votos
5

Su esquema de bloqueo está roto. Debe bloquear Foo.Requests()durante toda la duración del ciclo, no solo al eliminar un elemento. De lo contrario, el artículo podría perder validez en el medio de la operación de "procesar los datos" y la enumeración podría cambiar entre pasar de un elemento a otro. Y eso supone que no necesita insertar la colección durante este intervalo también. Si ese es el caso, realmente necesita volver a factorizar para usar una cola apropiada de productor / consumidor.

Respondida el 22/07/2009 a las 19:48
fuente por usuario

votos
2

El problema es la expresión

new List<string>(Foo.Requests)

dentro de su foreach, porque no está bajo un candado. Supongo que mientras .NET copia su colección de solicitudes en una nueva lista, la lista es modificada por otro hilo

Respondida el 22/07/2009 a las 20:03
fuente por usuario

votos
2

Para ser completamente honesto, sugeriría refactorizar eso. Está eliminando elementos del objeto al mismo tiempo que itera sobre eso. Su ciclo podría salir antes de que haya procesado todos los elementos.

Respondida el 22/07/2009 a las 19:50
fuente por usuario

votos
2

Tres cosas:
- No los pondría en la declaración for (each), pero fuera de ella.
- No bloquearía la colección real, sino un objeto estático local
- No puede modificar una lista / colección que está enumerando

Para obtener más información, consulte:
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);
}
Respondida el 22/07/2009 a las 19:48
fuente por usuario

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

Supongamos que tiene dos hilos ejecutando este código.

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

  • Thread1 comienza a procesar la lista.
  • Thread2 llama al constructor List, que toma un recuento para que se cree la matriz.
  • Thread1 cambia la cantidad de elementos en la lista.
  • Thread2 tiene una cantidad incorrecta de elementos.

Su esquema de bloqueo es incorrecto. Incluso está mal en el ejemplo de bucle for.

Debe bloquear cada vez que acceda al recurso compartido, incluso para leerlo o copiarlo. Esto no significa que deba bloquear durante toda la operación. Significa que todos los que comparten este recurso compartido deben participar en el esquema de bloqueo.

También considere la copia defensiva:

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

//now process local list todos

Aun así, todos aquellos que comparten Foo.Requests deben participar en el esquema de bloqueo.

Respondida el 22/07/2009 a las 20:08
fuente por usuario

votos
0

Sé que no es lo que pediste, pero solo por el bien de mi cordura, ¿representa lo siguiente la intención de tu código?

private object _locker = new object();

// ...

lock (_locker) {
    Foo.Requests.Clear();
}
Respondida el 22/07/2009 a las 23:08
fuente por usuario

votos
0

Está intentando eliminar objetos de la lista mientras itera a través de la lista. (OK, técnicamente, no estás haciendo esto, pero ese es el objetivo que estás tratando de lograr).

Así es como lo hace correctamente: mientras itera, construya otra lista de entradas que quiera eliminar. Simplemente construya otra lista (temp), coloque todas las entradas que desee eliminar de la lista original en la lista temporal.

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 );

Usando el método "removeAll" de la clase List.

Respondida el 22/07/2009 a las 20:23
fuente por usuario

Cookies help us deliver our services. By using our services, you agree to our use of cookies. Learn more