¿Cómo evita que IDisposable se propague a todas sus clases?

138

Comience con estas clases simples ...

Digamos que tengo un conjunto simple de clases como esta:

class Bus
{
    Driver busDriver = new Driver();
}

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

class Shoe
{
    Shoelace lace = new Shoelace();
}

class Shoelace
{
    bool tied = false;
}

A Bustiene a Driver, Drivertiene dos Shoes, cada uno Shoetiene a Shoelace. Todo muy tonto.

Agregue un objeto IDisposable a Shoelace

Más tarde decido que alguna operación en el Shoelacepodría ser multiproceso, así que agrego un EventWaitHandlepara que los hilos se comuniquen. Entonces Shoelaceahora se ve así:

class Shoelace
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    // ... other stuff ..
}

Implemento IDisposable en Cordón

Pero ahora FxCop de Microsoft se quejará: "Implemente IDisposable en 'Shoelace' porque crea miembros de los siguientes tipos de IDisposable: 'EventWaitHandle'".

Está bien, puedo aplicar IDisposableen Shoelacey mi clase se convierte en poco aseado este horrible desastre:

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    private bool disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~Shoelace()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                if (waitHandle != null)
                {
                    waitHandle.Close();
                    waitHandle = null;
                }
            }
            // No unmanaged resources to release otherwise they'd go here.
        }
        disposed = true;
    }
}

O (como lo señalaron los comentaristas) ya que en Shoelacesí mismo no tiene recursos no administrados, podría usar la implementación de disposición más simple sin necesidad de Dispose(bool)Destructor y:

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;

    public void Dispose()
    {
        if (waitHandle != null)
        {
            waitHandle.Close();
            waitHandle = null;
        }
        GC.SuppressFinalize(this);
    }
}

Mire con horror cómo se propaga IDisposable

Correcto, eso es todo arreglado. Pero ahora FxCop se quejará de que Shoecrea un Shoelace, por Shoelo que IDisposabletambién debe ser .

Y Drivercrea Shoeasí Driverdebe ser IDisposable. Y Buscrea Driverasí Busdebe ser IDisposabley así sucesivamente.

De repente, mi pequeño cambio Shoelaceme está causando mucho trabajo y mi jefe se pregunta por qué tengo que pagar Buspara hacer un cambio Shoelace.

La pregunta

¿Cómo evita esta propagación IDisposable, pero se asegura de que sus objetos no gestionados se eliminen correctamente?

GrahamS
fuente
9
Una pregunta excepcionalmente buena, creo que la respuesta es minimizar su uso e intentar mantener los ID desechables de alto nivel de corta duración con el uso, pero esto no siempre es posible (especialmente cuando esos ID desechables se deben a la interoperabilidad con dlls C ++ o similares). Busque las respuestas.
annakata
Bien Dan, he actualizado la pregunta para mostrar ambos métodos de implementación de IDisposable en Shoelace.
GrahamS
Normalmente desconfío de confiar en los detalles de implementación de otras clases para protegerme. No tiene sentido arriesgarlo si puedo evitarlo fácilmente. Tal vez soy demasiado cauteloso o tal vez pasé demasiado tiempo como programador en C, pero prefiero adoptar el enfoque irlandés: "Para estar seguro, estar seguro" :)
GrahamS
@Dan: La comprobación nula aún es necesaria para garantizar que el objeto en sí no se haya establecido en nulo, en cuyo caso la llamada a waitHandle.Dispose () arrojará una NullReferenceException.
Scott Dorman el
1
No importa qué, en realidad aún debe usar el método Dispose (bool) como se muestra en la sección "Implementar ID disponible en el cordón" ya que ese (menos el finalizador) es el patrón completo. El hecho de que una clase implemente IDisposable no significa que necesite un finalizador.
Scott Dorman el

Respuestas:

36

Realmente no puede "evitar" que se propague IDisposable. Algunas clases deben eliminarse, como AutoResetEvent, y la forma más eficiente es hacerlo en el Dispose()método para evitar la sobrecarga de los finalizadores. Pero este método debe llamarse de alguna manera, por lo que exactamente como en su ejemplo, las clases que encapsulan o contienen IDisposable deben eliminarlos, por lo que también deben ser desechables, etc. La única forma de evitarlo es:

  • evite usar clases IDisposable cuando sea posible, bloquee o espere eventos en lugares únicos, mantenga recursos caros en un solo lugar, etc.
  • créelos solo cuando los necesite y deséchelos justo después (el usingpatrón)

En algunos casos, IDisposable se puede ignorar porque admite un caso opcional. Por ejemplo, WaitHandle implementa IDisposable para admitir un Mutex con nombre. Si no se usa un nombre, el método Dispose no hace nada. MemoryStream es otro ejemplo, no utiliza recursos del sistema y su implementación Dispose tampoco hace nada. Pensar cuidadosamente si un recurso no administrado se está utilizando o no puede ser instructivo. También puede examinar las fuentes disponibles para las bibliotecas .net o usar un descompilador.

Grzenio
fuente
44
El consejo de que puede ignorarlo porque la implementación de hoy no hace nada es malo, porque una versión futura podría hacer algo importante en Dispose, y ahora tiene dificultades para rastrear la fuga.
Andy
1
"mantenga recursos caros", los desechables de ID no son necesariamente caros, de hecho, este ejemplo que usa un mango de espera uniforme es casi un "peso liviano" a medida que se obtiene.
markmnl
20

En términos de corrección, no puede evitar la propagación de IDisposable a través de una relación de objeto si un objeto padre crea y posee esencialmente un objeto hijo que ahora debe ser desechable. FxCop es correcto en esta situación y el padre debe ser IDisposable.

Lo que puede hacer es evitar agregar un IDisposable a una clase de hoja en su jerarquía de objetos. Esto no siempre es una tarea fácil, pero es un ejercicio interesante. Desde una perspectiva lógica, no hay razón para que un ShoeLace deba ser desechable. En lugar de agregar un WaitHandle aquí, también es posible agregar una asociación entre un ShoeLace y un WaitHandle en el punto en que se usa. La forma más simple es a través de una instancia de Diccionario.

Si puede mover WaitHandle a una asociación suelta a través de un mapa en el punto en que se utiliza WaitHandle, puede romper esta cadena.

JaredPar
fuente
2
Se siente muy extraño hacer una asociación allí. Este AutoResetEvent es privado para la implementación de Shoelace, por lo que exponerlo públicamente sería un error en mi opinión.
GrahamS
@GrahamS, no estoy diciendo exponerlo públicamente. Estoy diciendo que se puede mover al punto donde se atan los cordones de los zapatos. Quizás si atar los cordones de los zapatos es una tarea tan compleja que debería ser una clase de cordones de zapatos.
JaredPar
1
¿Podría ampliar su respuesta con algunos fragmentos de código JaredPar? Puede que esté ampliando un poco mi ejemplo, pero estoy imaginando que Shoelace crea e inicia el hilo de Tyer que espera pacientemente a waitHandle.WaitOne ().
GrahamS
1
+1 en esto. Creo que sería extraño que solo Shoelace fuera llamado simultáneamente, pero no los otros. Piense en lo que debería ser la raíz agregada. En este caso, debería ser Bus, en mi humilde opinión, aunque no estoy familiarizado con el dominio. Entonces, en ese caso, el bus debe contener el control de espera y todas las operaciones contra el bus y todos sus hijos se sincronizarán.
Johannes Gustafsson
16

Para evitar la IDisposablepropagación, debe intentar encapsular el uso de un objeto desechable dentro de un único método. Intenta diseñar de manera Shoelacediferente:

class Shoelace { 
  bool tied = false; 

  public void Tie() {

    using (var waitHandle = new AutoResetEvent(false)) {

      // you can even pass the disposable to other methods
      OtherMethod(waitHandle);

      // or hold it in a field (but FxCop will complain that your class is not disposable),
      // as long as you take control of its lifecycle
      _waitHandle = waitHandle;
      OtherMethodThatUsesTheWaitHandleFromTheField();

    } 

  }
} 

El alcance del controlador de espera está limitado al Tiemétodo, y la clase no necesita tener un campo desechable, por lo que no será necesario que sea desechable.

Dado que el identificador de espera es un detalle de implementación dentro del Shoelace, no debe cambiar de ninguna manera su interfaz pública, como agregar una nueva interfaz en su declaración. ¿Qué sucederá entonces cuando ya no necesite un campo desechable, eliminará la IDisposabledeclaración? Si piensa en la Shoelace abstracción , rápidamente se da cuenta de que no debería estar contaminada por dependencias de infraestructura, como IDisposable. IDisposabledebe reservarse para clases cuya abstracción encapsule un recurso que requiera una limpieza determinista; es decir, para clases donde la disposición es parte de la abstracción .

Jordão
fuente
1
Estoy completamente de acuerdo en que los detalles de implementación de Shoelace no deberían contaminar la interfaz pública, pero mi punto es que puede ser difícil de evitar. Lo que sugiere aquí no siempre sería posible: el propósito de AutoResetEvent () es comunicarse entre subprocesos, por lo que su alcance generalmente se extendería más allá de un solo método.
GrahamS
@GrahamS: por eso dije que intentara diseñarlo de esa manera. También puede pasar el desechable a otros métodos. Solo se descompone cuando las llamadas externas a la clase controlan el ciclo de vida del desechable. Actualizaré la respuesta en consecuencia.
Jordão
2
lo siento, sé que puedes pasar el desechable, pero todavía no veo que funcione. En mi ejemplo, AutoResetEventse usa para comunicarse entre diferentes subprocesos que operan dentro de la misma clase, por lo que tiene que ser una variable miembro. No puede limitar su alcance a un método. (por ejemplo, imagine que un subproceso simplemente se queda esperando un poco de trabajo bloqueándolo waitHandle.WaitOne(). El subproceso principal llama al shoelace.Tie()método, que simplemente hace waitHandle.Set()ay regresa de inmediato).
GrahamS
3

Esto es básicamente lo que sucede cuando mezclas Composición o Agregación con clases desechables. Como se mencionó, la primera salida sería refactorizar el waitHandle sin cordones.

Dicho esto, puede eliminar considerablemente el patrón Desechable cuando no tiene recursos no administrados. (Todavía estoy buscando una referencia oficial para esto).

Pero puede omitir el destructor y GC.SuppressFinalize (esto); y tal vez limpie un poco el virtual virtual Dispose (bool disposing).

Henk Holterman
fuente
Gracias Henk Si saqué la creación de waitHandle de Shoelace, entonces alguien todavía tiene que deshacerse de él en algún lugar, entonces, ¿cómo podría saber alguien que Shoelace ha terminado con eso (y que Shoelace no lo ha pasado a ninguna otra clase)?
GrahamS
Tendrás que mover no solo la creación de sino también la 'responsabilidad' del mango de espera. La respuesta de jaredPar tiene un comienzo interesante de una idea.
Henk Holterman
Este blog cubre la implementación de IDisposable, y en particular aborda cómo simplificar la tarea evitando mezclar recursos administrados y no administrados en un blog de
PaulS
3

Curiosamente, si Driverse define como anteriormente:

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

Luego, cuando Shoese realiza IDisposable, FxCop (v1.36) no se queja de que Drivertambién debería ser IDisposable.

Sin embargo, si se define así:

class Driver
{
    Shoe leftShoe = new Shoe();
    Shoe rightShoe = new Shoe();
}

entonces se quejará.

Sospecho que esto es solo una limitación de FxCop, en lugar de una solución, porque en la primera versión, las Shoeinstancias todavía están siendo creadas por Drivery todavía deben eliminarse de alguna manera.

GrahamS
fuente
2
Si Show implementa IDisposable, crearlo en un inicializador de campo es peligroso. Es interesante que FXCop no permita tales inicializaciones, ya que en C # la única forma de hacerlo de forma segura es con las variables ThreadStatic (francamente horribles). En vb.net, es posible inicializar objetos IDisposable de forma segura sin las variables ThreadStatic. El código sigue siendo feo, pero no tan horrible. Desearía que MS proporcionara una buena manera de usar dichos inicializadores de campo de forma segura.
supercat
1
@supercat: gracias. ¿Puedes explicar por qué es peligroso? Supongo que si se lanzara una excepción en uno de los Shoeconstructores, shoes¿quedaría en un estado difícil?
GrahamS
3
A menos que se sepa que un tipo particular de IDisposable se puede abandonar de forma segura, se debe garantizar que cada objeto que se crea se deseche. Si se crea un IDisposable en el inicializador de campo de un objeto pero se lanza una excepción antes de que el objeto esté completamente construido, el objeto y todos sus campos se abandonarán. Incluso si la construcción del objeto está envuelta en un método de fábrica que utiliza un bloque "probar" para detectar que la construcción falló, es difícil para la fábrica obtener referencias de los objetos que necesitan ser eliminados.
supercat
3
La única forma en que sé lidiar con eso en C # sería usar una variable ThreadStatic para mantener una lista de objetos que deberán eliminarse si el constructor actual arroja. Luego, haga que cada inicializador de campo registre cada objeto a medida que se crea, haga que la fábrica borre la lista si se completa con éxito, y use una cláusula "finalmente" para eliminar todos los elementos de la lista si no se borró. Ese sería un enfoque viable, pero las variables ThreadStatic son feas. En vb.net, uno podría usar una técnica similar sin necesidad de ninguna variable ThreadStatic.
supercat
3

No creo que haya una forma técnica de evitar que se propague IDisposable si mantiene su diseño tan estrechamente acoplado. Uno debería preguntarse si el diseño es correcto.

En su ejemplo, creo que tiene sentido que el zapato sea dueño del cordón, y tal vez, el conductor debería tener sus zapatos. Sin embargo, el autobús no debe ser dueño del conductor. Por lo general, los conductores de autobuses no siguen los autobuses hasta el depósito de chatarra :) En el caso de los conductores y los zapatos, los conductores rara vez hacen sus propios zapatos, lo que significa que realmente no los "poseen".

Un diseño alternativo podría ser:

class Bus
{
   IDriver busDriver = null;
   public void SetDriver(IDriver d) { busDriver = d; }
}

class Driver : IDriver
{
   IShoePair shoes = null;
   public void PutShoesOn(IShoePair p) { shoes = p; }
}

class ShoePairWithDisposableLaces : IShoePair, IDisposable
{
   Shoelace lace = new Shoelace();
}

class Shoelace : IDisposable
{
   ...
}

Desafortunadamente, el nuevo diseño es más complicado, ya que requiere clases adicionales para crear instancias y eliminar instancias concretas de zapatos y conductores, pero esta complicación es inherente al problema que se está resolviendo. Lo bueno es que los autobuses ya no necesitan ser desechables simplemente con el fin de deshacerse de los cordones de los zapatos.

Joh
fuente
2
Sin embargo, esto realmente no resuelve el problema original. Puede mantener el FxCop en silencio, pero algo debería deshacerse del Cordón.
AndrewS
Creo que todo lo que has hecho es mover el problema a otra parte. ShoePairWithDisposableLaces ahora posee Shoelace (), por lo que también debe hacerse IDisposable, entonces, ¿quién se deshace de los zapatos? ¿Vas a dejar esto a algún contenedor de IoC para manejar?
GrahamS
@AndrewS: De hecho, algo debe deshacerse de los conductores, zapatos y cordones, pero la eliminación no debe ser iniciada por los autobuses. @GrahamS: Tienes razón, estoy trasladando el problema a otra parte, porque creo que pertenece a otra parte. Por lo general, habría una clase de zapatos de instanciación, que también sería responsable de deshacerse de los zapatos. He editado el código para que ShoePairWithDisposableLaces también sea desechable.
Joh
@Joh: gracias. Como dices, el problema con moverlo a otro lado es que creas mucha más complejidad. Si ShoePairWithDisposableLaces es creado por alguna otra clase, entonces ¿no debería notificarse esa clase cuando el conductor haya terminado con sus zapatos, para que pueda deshacerse de ellos ()?
GrahamS
1
@Graham: sí, se necesitaría un mecanismo de notificación de algún tipo. Se podría utilizar una política de eliminación basada en recuentos de referencia, por ejemplo. Hay algo de complejidad añadida, pero como probablemente sabrás, "No hay tal cosa como un zapato gratis" :)
Joh
1

¿Qué tal usar la Inversión de Control?

class Bus
{
    private Driver busDriver;

    public Bus(Driver busDriver)
    {
        this.busDriver = busDriver;
    }
}

class Driver
{
    private Shoe[] shoes;

    public Driver(Shoe[] shoes)
    {
        this.shoes = shoes;
    }
}

class Shoe
{
    private Shoelace lace;

    public Shoe(Shoelace lace)
    {
        this.lace = lace;
    }
}

class Shoelace
{
    bool tied;
    private AutoResetEvent waitHandle;

    public Shoelace(bool tied, AutoResetEvent waitHandle)
    {
        this.tied = tied;
        this.waitHandle = waitHandle;
    }
}

class Program
{
    static void Main(string[] args)
    {
        using (var leftShoeWaitHandle = new AutoResetEvent(false))
        using (var rightShoeWaitHandle = new AutoResetEvent(false))
        {
            var bus = new Bus(new Driver(new[] {new Shoe(new Shoelace(false, leftShoeWaitHandle)),new Shoe(new Shoelace(false, rightShoeWaitHandle))}));
        }
    }
}
Darragh
fuente
Ahora lo ha convertido en el problema de las personas que llaman, y Shoelace no puede depender de que el control de espera esté disponible cuando lo necesite.
Andy
@andy ¿Qué quieres decir con "Cordón de zapato no puede depender de que el mango de espera esté disponible cuando lo necesita"? Se pasa al constructor.
Darragh
Algo más podría haber alterado el estado de los eventos automáticos antes de dárselo a Shoelace, y podría comenzar con el ARE en mal estado; algo más podría interferir con el estado del ARE mientras Shoelace está haciendo lo suyo, haciendo que Shoelace haga lo incorrecto. Es la misma razón por la que solo bloqueas a miembros privados. "En general, ... evite bloquear instancias más allá de su control de códigos" docs.microsoft.com/en-us/dotnet/csharp/language-reference/…
Andy
Estoy de acuerdo con solo bloquear a miembros privados. Pero parece que las manijas de espera son diferentes. De hecho, parece más útil pasarlos al objeto, ya que la misma instancia debe usarse para comunicarse a través de hilos. No obstante, creo que IoC proporciona una solución útil para el problema del OP.
Darragh
Dado que el ejemplo de OP tenía el manejador de espera como miembro privado, la suposición segura es que el objeto espera acceso exclusivo a él. Hacer que el identificador sea visible fuera de la instancia viola eso. Por lo general, un IoC puede ser bueno para cosas como esta, pero no cuando se trata de subprocesos.
Andy