Como você evita que o IDisposable se espalhe para todas as suas classes?

138

Comece com essas classes simples ...

Digamos que eu tenha um conjunto simples de classes como este:

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 Bustem a Driver, o Drivertem dois Shoes, cada Shoeum tem a Shoelace. Tudo muito bobo.

Adicione um objeto IDisposable ao Shoelace

Mais tarde, decido que alguma operação no Shoelacepoderia ser multithread, então adiciono um EventWaitHandlepara os threads se comunicarem. Então Shoelaceagora fica assim:

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

Implementar IDisposable no cadarço

Mas agora o FxCop da Microsoft irá reclamar: "Implemente IDisposable no 'Shoelace' porque cria membros dos seguintes tipos de IDisposable: 'EventWaitHandle'."

Ok, eu implementar IDisposableem Shoelacee minha classe pequeno puro torna-se essa bagunça horrível:

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

Ou (como apontado pelos comentaristas), como Shoelaceele próprio não possui recursos não gerenciados, eu poderia usar a implementação de descarte mais simples sem precisar do Dispose(bool)e Destructor:

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

Assista com horror como IDisposable se espalha

Certo, isso é fixo. Mas agora o FxCop irá reclamar que Shoecria um Shoelace, assim também Shoedeve ser IDisposable.

E Drivercria Shoeassim Driverdeve ser IDisposable. E Buscria Driverassim Busdeve ser IDisposablee assim por diante.

De repente, minha pequena mudança Shoelaceestá me causando muito trabalho e meu chefe está se perguntando por que preciso fazer o checkout Buspara fazer uma alteração Shoelace.

A questão

Como você evita essa propagação IDisposable, mas ainda garante que seus objetos não gerenciados sejam descartados corretamente?

GrahamS
fonte
9
Uma pergunta excepcionalmente boa, acredito que a resposta é minimizar o uso e tentar manter os IDisposables de alto nível com vida curta, mas isso nem sempre é possível (especialmente quando esses IDisposables devem interoperar com dlls C ++ ou similares). Olhe para as respostas.
31909 annakata
Ok Dan, atualizei a pergunta para mostrar os dois métodos de implementação do IDisposable no Shoelace.
21416 GrahamS
Normalmente, desconfio de confiar nos detalhes de implementação de outras classes para me proteger. Não faz sentido arriscar se eu puder impedi-lo facilmente. Talvez eu sou excesso de cautela ou talvez eu só passei muito tempo como um programador C, mas eu prefiro tomar a abordagem irlandês: "Com certeza, com certeza" :)
Grahams
@ Dan: A verificação nula ainda é necessária para garantir que o próprio objeto não tenha sido definido como nulo; nesse caso, a chamada para waitHandle.Dispose () lançará uma NullReferenceException.
Scott Dorman
1
Não importa o que aconteça, você ainda deve realmente usar o método Dispose (bool), como mostrado na seção "Implementar IDisposable on Shoelace", pois esse (menos o finalizador) é o padrão completo. Só porque uma classe implanta IDisposable não significa que precisa de um finalizador.
Scott Dorman

Respostas:

36

Você realmente não pode "impedir" que IDisposable se espalhe. Algumas classes precisam ser descartadas, AutoResetEvente a maneira mais eficiente é fazê-lo no Dispose()método para evitar a sobrecarga dos finalizadores. Mas esse método deve ser chamado de alguma forma, exatamente como no seu exemplo, as classes que encapsulam ou contêm IDisposable precisam descartá-las; portanto, também devem ser descartáveis ​​etc. A única maneira de evitá-lo é:

  • evite usar classes IDisposable sempre que possível, bloqueie ou aguarde eventos em locais únicos, mantenha recursos caros em um único local etc.
  • crie-os somente quando precisar deles e descarte-os logo após (o usingpadrão)

Em alguns casos, o IDisposable pode ser ignorado porque suporta um caso opcional. Por exemplo, WaitHandle implementa IDisposable para oferecer suporte a um Mutex nomeado. Se um nome não estiver sendo usado, o método Dispose não fará nada. MemoryStream é outro exemplo, ele não usa recursos do sistema e sua implementação Dispose também não faz nada. Pensar cuidadosamente se um recurso não gerenciado está sendo usado ou não pode ser instrutivo. Assim, é possível examinar as fontes disponíveis para as bibliotecas .net ou usar um descompilador.

Grzenio
fonte
4
O conselho de que você pode ignorá-lo porque a implementação hoje não faz nada é ruim, porque uma versão futura pode realmente fazer algo importante em Dispose, e agora é difícil rastrear o vazamento.
Andy
1
"mantenha recursos caros", os IDisposable não são necessariamente caros; de fato, este exemplo que usa uma alça de espera uniforme é sobre um "peso leve" que você obtém.
markmnl
20

Em termos de correção, não é possível impedir a propagação de IDisposable por meio de um relacionamento de objeto se um objeto pai criar e possuir essencialmente um objeto filho que agora deve ser descartável. O FxCop está correto nessa situação e o pai deve ser IDisposable.

O que você pode fazer é evitar adicionar um IDisposable a uma classe folha na sua hierarquia de objetos. Nem sempre é uma tarefa fácil, mas é um exercício interessante. De uma perspectiva lógica, não há razão para que um ShoeLace precise ser descartável. Em vez de adicionar um WaitHandle aqui, também é possível adicionar uma associação entre um ShoeLace e um WaitHandle no ponto em que é usado. A maneira mais simples é através de uma instância do Dictionary.

Se você pode mover o WaitHandle para uma associação frouxa por meio de um mapa no ponto em que o WaitHandle é realmente usado, você pode quebrar essa cadeia.

JaredPar
fonte
2
É muito estranho fazer uma associação lá. Este AutoResetEvent é privado para a implementação do Shoelace, portanto, expô-lo publicamente seria errado na minha opinião.
21416 GrahamS
@ GrahamS, não estou dizendo expô-lo publicamente. Estou dizendo que pode ser movido para o ponto em que os cadarços estão amarrados. Talvez se amarrar cadarços de sapato for uma tarefa tão complexa, deva ser uma classe de cadarço.
JaredPar
1
Você poderia expandir sua resposta com alguns trechos de código JaredPar? Eu posso esticar um pouco o meu exemplo, mas estou imaginando que o Shoelace cria e inicia o thread do Tyer, que espera pacientemente no waitHandle.WaitOne () O Shoelace chamaria waitHandle.Set () quando quisesse que o thread começasse a amarrar.
21411 GrahamS
1
+1 nisso. Eu acho que seria estranho que apenas o Shoelace fosse chamado simultaneamente, mas não os outros. Pense no que deve ser a raiz agregada. Neste caso, deve ser Bus, IMHO, embora eu não esteja familiarizado com o domínio. Portanto, nesse caso, o barramento deve conter a alça de espera e todas as operações contra o barramento e todos os seus filhos serão sincronizados.
Johannes Gustafsson
16

Para impedir a IDisposablepropagação, você deve tentar encapsular o uso de um objeto descartável dentro de um único método. Tente criar um design 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();

    } 

  }
} 

O escopo do identificador de espera é limitado ao Tiemétodo, e a classe não precisa ter um campo descartável e, portanto, não precisa ser descartável.

Como o identificador de espera é um detalhe de implementação dentro Shoelacedele, não deve mudar de forma alguma sua interface pública, como adicionar uma nova interface em sua declaração. O que acontecerá quando você não precisar mais de um campo descartável, você removerá a IDisposabledeclaração? Se você pensa na Shoelace abstração , percebe rapidamente que ela não deve ser poluída por dependências de infraestrutura, como IDisposable. IDisposabledeve ser reservado para classes cuja abstração encapsula um recurso que requer limpeza determinística; ou seja, para classes em que a disponibilidade é parte da abstração .

Jordão
fonte
1
Concordo plenamente que os detalhes da implementação do Shoelace não devem poluir a interface pública, mas meu argumento é que pode ser difícil evitar. O que você sugere aqui nem sempre é possível: o objetivo do AutoResetEvent () é se comunicar entre os threads, portanto seu escopo geralmente se estende além de um único método.
Grahams
@ GrahamS: é por isso que eu disse para tentar projetar dessa maneira. Você também pode passar o descartável para outros métodos. Ele só é interrompido quando chamadas externas para a classe controlam o ciclo de vida do descartável. Vou atualizar a resposta de acordo.
Jordão
2
desculpe, eu sei que você pode passar o descartável por aí, mas ainda não vejo isso funcionando. No meu exemplo, o AutoResetEventé usado para se comunicar entre diferentes segmentos que operam dentro da mesma classe, portanto, deve ser uma variável de membro. Você não pode limitar seu escopo a um método. (por exemplo, imagine que um thread simplesmente fique esperando por algum trabalho bloqueando waitHandle.WaitOne(). O thread principal chama o shoelace.Tie()método, que apenas executa waitHandle.Set()ae retorna imediatamente).
Grahams
3

É basicamente o que acontece quando você mistura Composição ou Agregação com classes descartáveis. Como mencionado, a primeira saída seria refatorar o waitHandle do cadarço.

Dito isto, você pode reduzir consideravelmente o padrão Descartável quando não possui recursos não gerenciados. (Ainda estou procurando uma referência oficial para isso.)

Mas você pode omitir o destruidor e GC.SuppressFinalize (this); e talvez limpe um pouco o vazio virtual Dispose (descarte booleano).

Henk Holterman
fonte
Obrigado Henk. Se eu tirei a criação de waitHandle do Shoelace, alguém ainda terá que descartá- lo em algum lugar. Como alguém saberia que o Shoelace terminou com ele (e que o Shoelace não o passou para outras classes)?
21416 GrahamS
Você precisará mover não apenas a Criação, mas também a 'responsabilidade pela' manivela. A resposta de jaredPar tem um começo interessante de uma ideia.
Henk Holterman
Este blog aborda a implementação de IDisposable e, em particular, aborda como simplificar a tarefa, evitando a mistura de recursos gerenciados e não gerenciados em um blog de
2009
3

Curiosamente, se Driveré definido como acima:

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

Então, quando Shoeé feito IDisposable, o FxCop (v1.36) não reclama que Drivertambém deveria ser IDisposable.

No entanto, se for definido assim:

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

então vai reclamar.

Suspeito que isso seja apenas uma limitação do FxCop, e não uma solução, porque na primeira versão as Shoeinstâncias ainda estão sendo criadas pelo Drivere ainda precisam ser descartadas de alguma forma.

GrahamS
fonte
2
Se Show implementa IDisposable, a criação em um inicializador de campo é perigosa. Interessante que o FXCop não permitiria tais inicializações, pois em C # a única maneira de fazê-las com segurança é com variáveis ​​ThreadStatic (absolutamente hediondas). No vb.net, é possível inicializar objetos IDisposable com segurança sem variáveis ​​ThreadStatic. O código ainda é feio, mas não tão hediondo. Desejo que o MS forneça uma boa maneira de usar esses inicializadores de campo com segurança.
supercat
1
@ supercat: obrigado. Você pode explicar por que é perigoso? Estou supondo que, se uma exceção fosse lançada em um dos Shoeconstrutores, shoesseria deixada em um estado difícil?
GRAHAMS
3
A menos que se saiba que um determinado tipo de IDisposable pode ser abandonado com segurança, deve-se garantir que todos os objetos criados sejam descartados. Se um IDisposable for criado no inicializador de campo de um objeto, mas uma exceção for lançada antes que o objeto seja completamente construído, o objeto e todos os seus campos serão abandonados. Mesmo que a construção do objeto seja envolvida em um método de fábrica que use um bloco "try" para detectar que a construção falhou, é difícil para a fábrica obter referências aos objetos que precisam ser descartados.
precisa
3
A única maneira que sei lidar com isso em C # seria usar uma variável ThreadStatic para manter uma lista de objetos que precisarão ser descartados se o construtor atual for lançado. Em seguida, cada inicializador de campo registre cada objeto conforme ele é criado, faça com que a fábrica limpe a lista se for concluída com êxito e use uma cláusula "finalmente" para Dispor todos os itens da lista, se não tiver sido limpa. Essa seria uma abordagem viável, mas as variáveis ​​ThreadStatic são feias. No vb.net, pode-se usar uma técnica semelhante sem precisar de nenhuma variável ThreadStatic.
Supercat
3

Eu não acho que exista uma maneira técnica de impedir que o IDisposable se espalhe se você mantiver seu design tão firmemente acoplado. Deve-se então perguntar se o design está correto.

No seu exemplo, acho que faz sentido que o sapato seja o dono do cadarço e, talvez, o motorista deva ser o dono dos sapatos. No entanto, o ônibus não deve ser o dono do motorista. Normalmente, os motoristas de ônibus não seguem os ônibus até o ferro-velho :) No caso de motoristas e sapatos, os motoristas raramente fazem seus próprios sapatos, o que significa que eles realmente não os "possuem".

Um design alternativo pode 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
{
   ...
}

Infelizmente, o novo design é mais complicado, pois exige classes extras para instanciar e descartar instâncias concretas de calçados e tênis, mas essa complicação é inerente ao problema que está sendo resolvido. O bom é que os ônibus não precisam mais ser descartáveis ​​simplesmente com a finalidade de descartar os cadarços.

Joh
fonte
2
Isso realmente não resolve o problema original. Pode manter o FxCop quieto, mas algo ainda deve descartar o cadarço.
AndrewS
Eu acho que tudo que você fez foi mudar o problema para outro lugar. ShoePairWithDisposableLaces agora possui Shoelace (), portanto, ele também deve ser tornado IDisposable - então quem descarta os sapatos? Você está deixando isso para algum contêiner de IoC para lidar?
Grahams
@ Andrews: De fato, algo ainda deve descartar motoristas, sapatos e cadarços, mas o descarte não deve ser iniciado por ônibus. @ GrahamS: Você está certo, estou mudando o problema para outro lugar, porque acredito que ele pertence a outro lugar. Normalmente, haveria uma classe de instanciação de sapatos, que também seria responsável pelo descarte de sapatos. Também editei o código para tornar o ShoePairWithDisposableLaces descartável.
Joh
@ Oh: Obrigado. Como você diz, o problema de movê-lo para outro lugar é que você cria muito mais complexidade. Se ShoePairWithDisposableLaces for criado por alguma outra classe, essa classe não teria que ser notificada quando o Driver terminar com seus Sapatos, para que possa Dispose () deles adequadamente?
Grahams
1
@ Graham: sim, seria necessário algum tipo de mecanismo de notificação. Uma política de descarte baseada em contagens de referência pode ser usada, por exemplo. Há alguma complexidade adicional, mas como você provavelmente sabe, "Não há tal coisa como um sapato livre" :)
Joh
1

Que tal usar Inversão de Controle?

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
fonte
Agora você tornou o problema dos chamadores, e o Shoelace não pode depender da alça de espera disponível quando necessário.
Andy
@andy O que você quer dizer com "Cadarço não pode depender da alça de espera disponível quando é necessário"? É passado para o construtor.
Darragh
Outra coisa pode ter mexido com o estado dos eventos automáticos antes de dar ao Shoelace e pode começar com o ARE em um estado ruim; outra coisa pode atrapalhar o estado do ARE enquanto o Shoelace está fazendo seu trabalho, fazendo com que o Shoelace faça a coisa errada. É o mesmo motivo pelo qual você bloqueia apenas membros privados. "Em geral, .. evite bloquear instâncias fora do controle de seus códigos" docs.microsoft.com/en-us/dotnet/csharp/language-reference/…
Andy
Concordo apenas em bloquear membros privados. Mas parece que as alças de espera são diferentes. De fato, parece mais útil passá-los para o objeto, pois a mesma instância precisa ser usada para se comunicar através de threads. No entanto, acho que a IoC fornece uma solução útil para o problema do OP.
Darragh
Dado que o exemplo dos OPs teve o handhandle como um membro privado, a suposição segura é que o objeto espera acesso exclusivo a ele. Tornar o identificador visível fora da instância viola isso. Normalmente, uma IoC pode ser boa para coisas como essa, mas não quando se trata de segmentação.
187 Andy Andy