Clase para gestionar Entrada/Salida

[WaaghMan]

Aquí os dejo la clase que estamos usando para gestionar la entrada/salida, para que se haga en un hilo aparte, ya sea para dejar en segundo plano o para esperar a que acabe y luego siga. También gestiona cargado y guardado (en el caso de guardado, cierra y reabre el contenedor para que se guarden los datos).

Probablemente la única cosa que no os compilaría es la referencia a Engine.GlobalStorageDevice, se trata de una variable estática que referencia al StorageDevice escogido al comienzo de la aplicación.

Los métodos más importantes son Exec ( que realiza la operación de entrada/salida y espera a que termine, por ejemplo para cargas importantes ) y Post (que la deja en segundo plano y prosigue la ejecución, por ejemplo para guardar cosas).

Lo siento por la falta de comentarios, no soy muy dado a hacer código legible  :P

Código [Seleccionar] Expand

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework.Storage;
using System.Threading;
using System.Collections;
using Barrage.Global;

namespace Barrage.GameUtils.Storage
{
    public class StorageManager
    {
        static string containerName = "";
        public static string ContainerName { set { containerName = value; } }

        public delegate void IOOperation(StorageContainer container, object parameters);
        public enum IOType
        {
            Read,
            Write
        }
        public enum IOPriority
        {
            High,
            Low
        }
        static StorageManager instance = null;
        public static StorageManager Instance { get { if (instance == null) instance = new StorageManager(); return instance; } }

        struct IOOperationData
        {
            public IOOperation Operation;
            public IOType Type;
            public AutoResetEvent taskFinishedEvent;
            public object parameters;
            public IOOperationData(IOOperation operation, object parameters, IOType type)
            {
                this.Operation = operation;
                this.Type = type;
                this.parameters = parameters;
                taskFinishedEvent = null;
            }

            public IOOperationData(IOOperation operation, object parameters, IOType type, AutoResetEvent taskFinishedEvent)
            {
                this.Operation = operation;
                this.Type = type;
                this.parameters = parameters;
                this.taskFinishedEvent = taskFinishedEvent;
            }
        }

        AutoResetEvent taskFinishedEvent;

        StorageManager()
        {
            if (containerName.Length == 0)
                throw new Exception("Container name has not been set up!");
            newItemEvent = new AutoResetEvent(false);
            taskFinishedEvent = new AutoResetEvent(false);
            workerThread = new Thread(new ThreadStart(work));
            workerThread.IsBackground = true;
            workerThread.Start();
        }

        // Execute and wait for an IO Task to be finished. Warning, this could fail if more than one Exec are run at the same time
        public void Exec(IOOperation operation, object parameters, IOType ioType)
        {
            lock (((ICollection)highPriorityTasks).SyncRoot)
            {
                highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType, taskFinishedEvent));
                newItemEvent.Set();
            }
            // Wait for it to be finished
            taskFinishedEvent.WaitOne();
        }

        public void Post(IOOperation operation, object parameters, IOType ioType, IOPriority ioPriority)
        {
            Queue<IOOperationData> list = null;
            switch (ioPriority)
            {
                case IOPriority.High:
                    list = highPriorityTasks;
                    break;
                case IOPriority.Low:
                default:
                    list = lowPriorityTasks;
                    break;
            }
            lock (((ICollection)list).SyncRoot)
            {
                highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType));
                newItemEvent.Set();
            }
        }

        Thread workerThread = null;

        StorageContainer container = null;

        Queue<IOOperationData> lowPriorityTasks = new Queue<IOOperationData>();
        Queue<IOOperationData> highPriorityTasks = new Queue<IOOperationData>();

        AutoResetEvent newItemEvent;

        void work()
        {
#if XBOX
            // Set processor affinity to core 5
            Thread.CurrentThread.SetProcessorAffinity(new[] { 4 });
#endif
            openContainer();
            try
            {
                while (true)
                {
                    if (highPriorityTasks.Count + lowPriorityTasks.Count == 0)
                    {
                        newItemEvent.WaitOne();
                        // Evita que si el evento estaba activado pero ya se había tratado el tema anteriormente,
                        // no intentemos procesar una tarea que ya estaba hecha.
                        if (highPriorityTasks.Count + lowPriorityTasks.Count == 0)
                            continue;
                    }
                    IOOperationData operation;
                    if (highPriorityTasks.Count > 0)
                        lock (((ICollection)highPriorityTasks).SyncRoot)
                        {
                            operation = highPriorityTasks.Dequeue();
                        }
                    else
                        lock (((ICollection)lowPriorityTasks).SyncRoot)
                        {
                            operation = lowPriorityTasks.Dequeue();
                        }
                    performOperation(ref operation);
                }
            }
            catch (ThreadAbortException)
            {
                closeContainer();
            }
            catch (Exception)
            {
                closeContainer();
            }
        }

        void performOperation(ref IOOperationData operationData)
        {
            try
            {
                checkContainer();
               
                operationData.Operation.Invoke(container, operationData.parameters);
                if (operationData.Type == IOType.Write)
                    reopenContainer();
            }
            catch (Exception)
            {

            }
            if (operationData.taskFinishedEvent != null)
                operationData.taskFinishedEvent.Set();
        }

        void checkContainer()
        {
            if ((Engine.GlobalStorageDevice == null) || (!Engine.GlobalStorageDevice.IsConnected))
            {
                if (container != null)
                {
                    container.Dispose();
                    container = null;
                }
            }
            else if (container == null)
                openContainer();
        }

        void openContainer()
        {
            if (container == null)
            {
                if ((Engine.GlobalStorageDevice != null) && (Engine.GlobalStorageDevice.IsConnected))
                    try
                    {
                        container = Engine.GlobalStorageDevice.OpenContainer(containerName);
                    }
                    catch (Exception)
                    {
                        container = null;
                    }
            }
        }

        void closeContainer()
        {
            if (container != null)
            {
                container.Dispose();
                container = null;
            }
        }

        void reopenContainer()
        {
            if ((Engine.GlobalStorageDevice == null) || (!Engine.GlobalStorageDevice.IsConnected))
                return;
            closeContainer();
            openContainer();
        }
    }
}

Espero que os resulte de ayuda o de interés. Cualquier duda o sugerencia será bien recibida.

[Vicente]

Hola!

a ver si luego tengo tiempo para leer el código con más calma, pero un pequeño "cambio" que podéis hacer es el siguiente: tenéis un delegado con la siguiente firma:

public delegate void IOOperation(StorageContainer container, object parameters);

Realmente a partir de .NET 3.5 esto se puede simplificar si usais un delegado Action, así que donde estéis pasando o usando una IOOperation podéis cambiarlo por:

Action<StorageContainer, object>

Que representa un delegado que no devuelve nada y recibe dos parámetros, el primero de tipo StorageContainer y el segundo un object :)

Un saludo!

Vicente

[WaaghMan]

Pues tienes razón. Aun así, creo que al especificar los delegados de esa manera, se hace reserva de memoria cada vez que se usan, y en la 360 mejor si se puede evitar. Pero procuraré acordarme de hacer la prueba

[Vicente]

Mmm, deberían ser equivalentes, es más, si cambias las declaraciones de tu delegado por el de Action debería todo compilar sin hacer nada de nada.

Creo que lo que tu te refieres es que alguien cumpla el delagado usando una lambda, algo como:

(a, b) => "código variado";

Ahí estás generando un nuevo delegado cada vez que llamas a ese método, lo cual en un bucle o algo así en la 360 sí puede ser una historia. Pero el cambio a Action es simplemente para ahorrarte el tener que inventar un tipo nuevo :)

[Vicente]

He mirado un poco más el código, tienes una movida del quince con los hilos :S A ver si esta tarde me puedo sentar un rato y reescribirlo :S

[Vicente]

DISCLAIMER: no he podido probar el código, creo que está correcto, pero podría no estarlo, lo importante es la idea.

He estado jugando un poco con el código y cambiando algunas cosillas. Actualmente tal y como lo tenéis, existe la posibilidad de que funcione mal dado que os habéis liado con el tema de los hilos :( Pero bueno, voy a explicar poco a poco como lo he ido cambiando a ver si os ayuda :)

Lo primero es que esta clase parece un Singleton, pero la forma de implementar el patrón no es la recomendada en C# y podría darse el caso de que se creen dos singletones por como lo habéis escrito. El problema es que tenéis el new StorageContainer() dentro del get de Instancia, y eso no está garantizado que solo se ejecute una vez, para hacer un singleton correcto ese new debería ejecutarse dentro del constructor estático del tipo.

Tenéis una explicación más detallada en mi blog: http://kartones.net/blogs/jadengine/archive/2009/05/20/el-patr-243-n-singleton-en-c.aspx Y aún mucho más detallada en estos dos artículos de John Skeet: http://www.yoda.arachsys.com/csharp/singleton.html y http://www.yoda.arachsys.com/csharp/beforefieldinit.html (aunque en C# 4.0 cambia un pelo el tema del BeforeFieldInit).

Da la impresión que el uso de vuestra clase es algo del estilo:

Código [Seleccionar] Expand


StorageManager.ContainerName = "nombre";
StorageManager.Instancia.VamosAHacerCosas();


Donde al construir comprobáis que realmente tenéis algún string en ContainerName que es algo que luego usaréis en el método work al llamar a openContainer. Esta forma de funcionar tiene varias cosas "feas" (esto es un poco mezcla de gusto personal y las guías de diseño):

- Tenéis una propiedad write-only (con solo un set). Esto aunque el lenguaje lo permite no está recomendado, se recomienda usar un método.
- Hacéis demasiadas cosas dentro del constructor :S

Mi idea ha sido cambiar esto para que el uso sea algo así:

Código [Seleccionar] Expand


StorageManager.Instancia.Start("nombre");


Esto me permite un montón de cosas: puedo implementar el patrón singleton de manera correcta, me cargo la propiedad write-only, y me encargo de comprobar el parámetro y lanzar el hilo separado de la construcción de la instancia. Creo que queda más bonito en general.

Vamos a ver el código que llevamos hasta este momento:

Código [Seleccionar] Expand


public class StorageManager2
{
    private static readonly StorageManager2 instance;

    private string containerName;
    private StorageContainer container;
    private ConcurrentQueue<IOOperationData> lowPriorityTasks;
    private ConcurrentQueue<IOOperationData> highPriorityTasks;
    private AutoResetEvent newItemEvent;
    private AutoResetEvent taskFinishedEvent;

    static StorageManager2()
    {
        StorageManager2.instance = new StorageManager2();
    }

    private StorageManager2()
    {
        this.lowPriorityTasks = new ConcurrentQueue<IOOperationData>();
        this.highPriorityTasks = new ConcurrentQueue<IOOperationData>();
        this.newItemEvent = new AutoResetEvent(false);
        this.taskFinishedEvent = new AutoResetEvent(false);
    }

    public static StorageManager2 Instance
    {
        get
        {
            return instance;
        }
    }

    public void Start(string containerName)
    {
        if (string.IsNullOrEmpty(containerName))
        {
            throw new ApplicationException("Container name can't be null or empty");
        }

        this.containerName = containerName;
        Thread workerThread = new Thread(() => this.Work());
        workerThread.IsBackground = true;
        workerThread.Start();
    }
}


Supongo que el código se entiende más o menos bien a pesar de los cambios. Esto nos lleva al segundo cambio "grande" en vuestra clase y el fallo que teníais con los hilos: si os fijáis, en vez de usar Queue<T> uso una clase llamada ConcurrentQueue<T>. Esta clase es una clase que me he hecho para manejar mejor el tema de la sincronización de hilos y he elegido ese nombre porque esa clase existe en el Framework 4.0 (http://msdn.microsoft.com/en-us/library/dd267265%28VS.100%29.aspx), así que si algún día XNA tenemos XNA en 4.0, solo tendréis que cambiar la referencia. Si no os gusta, pues elegís otro nombre y listo :p

Vamos al problema de vuestro código: vuestro fallo es que hacéis lock sobre Queue o Dequeue, pero no sobre Count. Es posible que se compruebe en un if si Count es mayor que 0, que salga que sí, y cuando se entre a hacer el Dequeue otro hilo haya hecho lo mismo y no haya nada que sacar de la cola.

En general, la programación multihilo es muy jodida, incluso en este caso que es "sencillo" fijaos que despiste más tonto. La solución sería meter tanto la comprobación de Count como el Dequeue en el mismo lock, pero realmente eso hace el código muy feo. Creo que queda bastante mejor haceros una clase auxiliar que se encargue de la sincronización como esta:

Código [Seleccionar] Expand


using System.Collections.Generic;

public class ConcurrentQueue<T>
{
    private object syncObj;
    private Queue<T> internalQueue;

    public ConcurrentQueue()
    {
        this.syncObj = new object();
        this.internalQueue = new Queue<T>();
    }

    public void Enqueue(T item)
    {
        lock (this.syncObj)
        {
            this.internalQueue.Enqueue(item);
        }
    }

    public bool TryDequeue(out T item)
    {
        lock (this.syncObj)
        {
            if (this.internalQueue.Count == 0)
            {
                item = default(T);
                return false;
            }

            item = this.internalQueue.Dequeue();
            return true;
        }
    }
}


Esto me lleva al tercer punto sobre el código: hay que pensar un poco sobre el "framework". Fijaos en que para hacer los locks hacéis:

Código [Seleccionar] Expand


lock (((ICollection)highPriorityTasks).SyncRoot)


¿Realmente no os parece horrendo a simple vista ese código? Es decir, si te vas a la MSDN se ve bien claro que Queue implementa ICollection, con lo que debería ser fácil usar la propiedad SyncRoot, pero cuando miras la documentación a fondo te das cuenta que esa propiedad está implementada de forma explícita, es decir que para usarla hay que hacer lo que estáis haciendo, castear a ICollection. ¿No da la impresión que si se quisiera que la gente usara SyncRoot para sincronizar se habría implementado de forma implícita para evitar ese cast? ¿No parece que están escondiendo la propiedad a propósito para evitar que la gente la use?

SyncRoot es como se pensó en .NET 1.0 el hacer sincronización en colecciones, pero realmente, como se pensó la sincronización en .NET 1.0 es un FAIL de los grandes (el libro CLR via C# lo explica en detalle, pero vamos, quedaros con la idea que la cagaron pero bien). En .NET 2.0 intentaron arreglar las cosas haciendo más difícil el uso de SyncRoot y añadiendo nuevas clases de sincronización. En .NET 3.5 han seguido añadiendo nuevas cositas y al final todo desemboca en la TPL (Tasks Parallel Library) de .NET 4.0.

Actualmente si se va a usar un lock, la recomendación es usar un objeto privado a la clase y hacer los locks sobre este, que es simplemente lo que hace la clase ConcurrentQueue, y además proporciona una manera de ejecutar un Count y un Dequeue de forma atómica con TryDequeue (realmente he traducido la idea que representa la clase de .NET 4.0, no es que haya sido muy original).

Así que esto nos permite reescribir los métodos Exec, Post y work de la siguiente manera:

Código [Seleccionar] Expand


public void Exec(Action<StorageContainer, object> operation, object parameters, IOType ioType)
{
    this.highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType, this.taskFinishedEvent));
    this.newItemEvent.Set();

    // Wait for it to be finished
    this.taskFinishedEvent.WaitOne();
}

public void Post(Action<StorageContainer, object> operation, object parameters, IOType ioType)
{
    this.highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType));
    this.newItemEvent.Set();
}

private void Work()
{
#if XBOX
    // Set processor affinity to core 5
    Thread.CurrentThread.SetProcessorAffinity(new[] { 4 });
#endif
    this.OpenContainer();

    try
    {
        while (true)
        {
            IOOperationData operation;
            if (this.highPriorityTasks.TryDequeue(out operation))
            {
                this.PerformOperation(operation);
            }
            else if (this.lowPriorityTasks.TryDequeue(out operation))
            {
                this.PerformOperation(operation);
            }
            else
            {
                this.newItemEvent.WaitOne();
            }
        }
    }
    catch (ThreadAbortException)
    {
        this.CloseContainer();
    }
    catch (Exception)
    {
        this.CloseContainer();
    }
}


(realmente no tengo claro que hacía el switch de Post, creo que nada y por eso me lo he cargado)
(creo que mi método Work es equivalente a vuestro método work, pero no acabo de entender vuestro primer if con las dos sumas contra 0 :S)

Y estos son los cambios "grandes" a la clase, lo demás son cambios un poco estéticos:

- Las clases pueden contener otras clases y enums, pero por estilo, si algo se puede usar desde fuera (como es el caso del enum IOType, debería estar fuera de la clase.
- Me he cargado vuestro delegado personalizado y en vez de eso uso el equivalente Action<StorageContainer, object>, como se ve en Exec y Post.
- Habéis implementado IOOperationData como un struct y no como una clase. En general hacer un struct que sean más de 3 ó 4 doubles es buscarse problemas de rendimiento. Si va a contener tanta información debería ser una clase.
- Y luego cambios varios para contentar a StyleCop.

Aquí os dejo el código completo:

Código [Seleccionar] Expand


namespace Barrage.GameUtils.Storage
{
    using System;
    using System.Threading;
    using Barrage.Global;
    using Microsoft.Xna.Framework.Storage;

    public enum IOType
    {
        Read,
        Write
    }

    public class StorageManager2
    {
        private static readonly StorageManager2 instance;

        private string containerName;
        private StorageContainer container;
        private ConcurrentQueue<IOOperationData> lowPriorityTasks;
        private ConcurrentQueue<IOOperationData> highPriorityTasks;
        private AutoResetEvent newItemEvent;
        private AutoResetEvent taskFinishedEvent;

        static StorageManager2()
        {
            StorageManager2.instance = new StorageManager2();
        }

        private StorageManager2()
        {
            this.lowPriorityTasks = new ConcurrentQueue<IOOperationData>();
            this.highPriorityTasks = new ConcurrentQueue<IOOperationData>();
            this.newItemEvent = new AutoResetEvent(false);
            this.taskFinishedEvent = new AutoResetEvent(false);
        }

        public static StorageManager2 Instance
        {
            get
            {
                return instance;
            }
        }

        public void Start(string containerName)
        {
            if (string.IsNullOrEmpty(containerName))
            {
                throw new ApplicationException("Container name can't be null or empty");
            }

            this.containerName = containerName;
            Thread workerThread = new Thread(() => this.Work());
            workerThread.IsBackground = true;
            workerThread.Start();
        }

        public void Exec(Action<StorageContainer, object> operation, object parameters, IOType ioType)
        {
            this.highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType, this.taskFinishedEvent));
            this.newItemEvent.Set();

            // Wait for it to be finished
            this.taskFinishedEvent.WaitOne();
        }

        public void Post(Action<StorageContainer, object> operation, object parameters, IOType ioType)
        {
            this.highPriorityTasks.Enqueue(new IOOperationData(operation, parameters, ioType));
            this.newItemEvent.Set();
        }

        private void Work()
        {
#if XBOX
    // Set processor affinity to core 5
    Thread.CurrentThread.SetProcessorAffinity(new[] { 4 });
#endif
            this.OpenContainer();

            try
            {
                while (true)
                {
                    IOOperationData operation;
                    if (this.highPriorityTasks.TryDequeue(out operation))
                    {
                        this.PerformOperation(operation);
                    }
                    else if (this.lowPriorityTasks.TryDequeue(out operation))
                    {
                        this.PerformOperation(operation);
                    }
                    else
                    {
                        this.newItemEvent.WaitOne();
                    }
                }
            }
            catch (ThreadAbortException)
            {
                this.CloseContainer();
            }
            catch (Exception)
            {
                this.CloseContainer();
            }
        }

        private void PerformOperation(IOOperationData operationData)
        {
            try
            {
                this.CheckContainer();

                operationData.Operation.Invoke(this.container, operationData.Parameters);
                if (operationData.Type == IOType.Write)
                {
                    this.ReopenContainer();
                }
            }
            catch (Exception)
            {
            }

            if (operationData.TaskFinishedEvent != null)
            {
                operationData.TaskFinishedEvent.Set();
            }
        }

        private void OpenContainer()
        {
            if (this.container == null)
            {
                if ((Engine.GlobalStorageDevice != null) && Engine.GlobalStorageDevice.IsConnected)
                {
                    try
                    {
                        this.container = Engine.GlobalStorageDevice.OpenContainer(this.containerName);
                    }
                    catch (Exception)
                    {
                        this.container = null;
                    }
                }
            }
        }

        private void CloseContainer()
        {
            if (this.container != null)
            {
                this.container.Dispose();
                this.container = null;
            }
        }

        private void ReopenContainer()
        {
            if ((Engine.GlobalStorageDevice == null) || (!Engine.GlobalStorageDevice.IsConnected))
            {
                return;
            }

            this.CloseContainer();
            this.OpenContainer();
        }

        private void CheckContainer()
        {
            if ((Engine.GlobalStorageDevice == null) || (!Engine.GlobalStorageDevice.IsConnected))
            {
                this.CloseContainer();
            }
            else if (this.container == null)
            {
                this.OpenContainer();
            }
        }

        private class IOOperationData
        {
            public IOOperationData(Action<StorageContainer, object> operation, object parameters, IOType type)
            {
                this.Operation = operation;
                this.Type = type;
                this.Parameters = parameters;
                this.TaskFinishedEvent = null;
            }

            public IOOperationData(Action<StorageContainer, object> operation, object parameters, IOType type, AutoResetEvent taskFinishedEvent)
            {
                this.Operation = operation;
                this.Type = type;
                this.Parameters = parameters;
                this.TaskFinishedEvent = taskFinishedEvent;
            }

            public Action<StorageContainer, object> Operation { get; private set; }

            public object Parameters { get; private set; }

            public IOType Type { get; private set; }

            public AutoResetEvent TaskFinishedEvent { get; private set; }
        }
    }
}


Espero que os haya parecido interesante (y que funcione, creo que debería pero con hilos nunca se sabe :p). Un saludo!

Vicente

[WaaghMan]

Muchas gracias por toda esa información, le echaré un ojo con más detalle más adelante.

Muchas de las cosas referentes al productor/consumidor están cogidas de un artículo de codeplex, supongo que estaba hecho para .NET 1.0  ^_^'

Algunas cosas rápidas:

* No tenía ni idea de lo del constructor estático, lo usaré a partir de ahora.
* Aún no tengo muy claro si el lock realmente tiene efecto en la Xbox 360, me parece haber leido en algún sitio que no.
* En el Post resulta que había un bug: Se debería introducir en la cola de alta o baja prioridad según el valor del switch, pero no se estaba haciendo.
* Lo de la suma en el if era para evitar que se entrase cuando había algún elemento pendiente tras ejecutar una operación, pero está claro que lo que tú has puesto es mucho más elegante  ::)



El IOOperationData se usa como struct para evitar el uso de news y tal, de todas maneras sólo son 4 referencias así que ocupa poco.

[Vicente]

Hola,

- El lock si debería funcionar en la XBox. Realmente un lock se traduce a Monitor.Enter y Monitor.Exit, y ambos métodos están soportados por XNA y el CF: http://msdn.microsoft.com/en-us/library/de0542zz.aspx Si encuentras otra cosa ponlo por favor :)
- Lo del IOOperation como struct sigue estando mal en mi opinión, realmente no estáis ganando nada con eso. A ver si encuentro un par de artículos del tema de Rico Mariani (aunque son muuuu técnicos).
- Lo del post pues nada, se cambia y listo :p Yo es que no le veía sentido :S

Un saludo!

Vicente