Как предотвратить распространение IDisposable на все ваши классы?

138

Начните с этих простых занятий ...

Скажем, у меня есть простой набор таких классов:

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 Busимеет a Driver, Driverимеет два Shoes, каждый Shoeимеет a Shoelace. Все очень глупо.

Добавить объект IDisposable в шнурки

Позже я решил, что некоторые операции над объектом Shoelaceмогут быть многопоточными, поэтому я добавляю, EventWaitHandleчтобы потоки взаимодействовали с ним. Итак, Shoelaceтеперь это выглядит так:

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

Реализуйте IDisposable на шнурках

Но теперь Microsoft FxCop будет жаловаться: «Реализуйте IDisposable на« Shoelace », потому что он создает элементы следующих типов IDisposable:« EventWaitHandle ».

Хорошо, я реализую IDisposableна Shoelaceи мой аккуратный класс немного становится этим ужасным беспорядком:

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

Или (как указано комментаторами), поскольку Shoelaceсам по себе не имеет неуправляемых ресурсов, я мог бы использовать более простую реализацию удаления без необходимости использования Dispose(bool)и Деструктора:

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

С ужасом наблюдайте, как IDisposable распространяется

Правильно, это исправлено. Но теперь FxCop будет жаловаться на то, что Shoeсоздает Shoelace, значит Shoe, IDisposableтоже.

И Driverтворит Shoeтак и Driverдолжно быть IDisposable. И Busтворит Driverтак Busдолжно быть IDisposableи так далее.

Внезапно мое небольшое изменение в Shoelaceвызывает у меня много работы, и мой босс задается вопросом, почему мне нужно Busоформить заказ, чтобы внести изменения Shoelace.

Вопрос

Как предотвратить такое распространение IDisposable, но при этом обеспечить правильную утилизацию неуправляемых объектов?

GrahamS
источник
9
Исключительно хороший вопрос, я считаю, что ответ - свести к минимуму их использование и попытаться сохранить недолговечные IDisposables высокого уровня с использованием, но это не всегда возможно (особенно когда эти IDisposables связаны с взаимодействием с библиотеками C ++ или аналогичными). Посмотрите ответы.
annakata
Хорошо, Дэн, я обновил вопрос, чтобы показать оба метода реализации IDisposable на Shoelace.
GrahamS
Обычно я опасаюсь полагаться на детали реализации других классов, чтобы защитить себя. Нет смысла рисковать, если я легко могу это предотвратить. Может быть, я слишком осторожен, или, может быть, я просто слишком долго работал программистом на C, но я бы предпочел ирландский подход: «Конечно, чтобы быть уверенным» :)
GrahamS
@Dan: проверка на null по-прежнему необходима, чтобы убедиться, что для самого объекта не установлено значение null, и в этом случае вызов waitHandle.Dispose () вызовет исключение NullReferenceException.
Скотт Дорман,
1
Несмотря ни на что, вы все равно должны использовать метод Dispose (bool), как показано в разделе «Внедрить IDisposable на шнурках», поскольку он (без финализатора) является полным шаблоном. Тот факт, что класс реализует IDisposable, не означает, что ему нужен финализатор.
Скотт Дорман,

Ответы:

36

Вы не можете "предотвратить" распространение IDisposable. Некоторые классы нужно удалить, например AutoResetEvent, и наиболее эффективный способ - сделать это в Dispose()методе, чтобы избежать накладных расходов на финализаторы. Но этот метод должен быть вызван каким-то образом, поэтому точно так же, как в вашем примере классы, которые инкапсулируют или содержат IDisposable, должны избавляться от них, поэтому они также должны быть одноразовыми и т. Д. Единственный способ избежать этого - это:

  • избегайте использования классов IDisposable, где это возможно, блокируйте или ждите событий в отдельных местах, храните дорогие ресурсы в одном месте и т. д.
  • создавайте их только тогда, когда они вам нужны, и утилизируйте их сразу после ( usingшаблона)

В некоторых случаях IDisposable можно игнорировать, поскольку он поддерживает необязательный регистр. Например, WaitHandle реализует IDisposable для поддержки именованного Mutex. Если имя не используется, метод Dispose ничего не делает. Другой пример - MemoryStream, он не использует системные ресурсы, а его реализация Dispose также ничего не делает. Тщательное обдумывание того, используется ли неуправляемый ресурс или нет, может быть полезным. Так же можно изучить доступные источники для библиотек .net или с помощью декомпилятора.

Грзенио
источник
4
Совет, что вы можете проигнорировать это, потому что реализация сегодня ничего не делает, плох, потому что будущая версия может действительно делать что-то важное в Dispose, и теперь вам трудно отследить утечку.
Энди
1
«держать дорогие ресурсы», IDisposable не обязательно являются дорогостоящими, на самом деле этот пример, в котором используется равномерный дескриптор ожидания, имеет «легкий вес».
markmnl
20

С точки зрения корректности, вы не можете предотвратить распространение IDisposable через объектные отношения, если родительский объект создает дочерний объект, который теперь должен быть одноразовым, и фактически владеет им. FxCop верен в этой ситуации, и родитель должен быть IDisposable.

Что вы можете сделать, так это избежать добавления IDisposable к конечному классу в вашей иерархии объектов. Это не всегда легкая задача, но это интересное упражнение. С логической точки зрения нет причин, по которым шнурки ShoeLace должны быть одноразовыми. Вместо добавления здесь WaitHandle можно также добавить связь между ShoeLace и WaitHandle в том месте, где она используется. Самый простой способ - использовать экземпляр Dictionary.

Если вы можете переместить WaitHandle в свободную ассоциацию через карту в точке фактического использования WaitHandle, вы можете разорвать эту цепочку.

ДжаредПар
источник
2
Очень странно создавать там ассоциации. Это AutoResetEvent является частным для реализации Shoelace, поэтому, на мой взгляд, было бы неправильно раскрывать его публично.
GrahamS
@GrahamS, я не говорю обнародовать это. Я говорю, можно ли его переместить в точку завязывания шнурков. Возможно, если завязать шнурки для обуви - такая сложная задача, они должны быть классом шнурков.
JaredPar
1
Не могли бы вы дополнить свой ответ некоторыми фрагментами кода JaredPar? Я могу немного растянуть свой пример, но я представляю, что Shoelace создает и запускает поток Tyer, который терпеливо ждет в waitHandle.WaitOne () Shoelace будет вызывать waitHandle.Set (), когда он хочет, чтобы поток начал связываться.
GrahamS
1
+1 по этому поводу. Думаю, было бы странно, что одновременно вызывались бы только Shoelace, а не другие. Подумайте, каким должен быть совокупный корень. В этом случае это должен быть Bus, IMHO, хотя я незнаком с доменом. Итак, в этом случае шина должна содержать дескриптор ожидания, и все операции с шиной и всеми ее дочерними элементами будут синхронизированы.
Йоханнес Густафссон
16

Чтобы предотвратить IDisposableраспространение, вы должны попытаться инкапсулировать использование одноразового объекта внутри одного метода. Попробуйте оформить Shoelaceиначе:

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

    } 

  }
} 

Область действия дескриптора ожидания ограничена Tieметодом, и класс не обязательно должен иметь одноразовое поле, и поэтому не должен быть одноразовым.

Поскольку дескриптор ожидания является деталью реализации внутри Shoelace, он не должен каким-либо образом изменять свой открытый интерфейс, например, добавляя новый интерфейс в его объявление. Что будет тогда, когда одноразовое поле вам больше не понадобится, вы удалите IDisposableобъявление? Если вы подумаете об Shoelace абстракции , вы быстро поймете, что она не должна загрязняться зависимостями инфраструктуры, например IDisposable. IDisposableдолжны быть зарезервированы для классов, абстракция которых инкапсулирует ресурс, требующий детерминированной очистки; т. е. для классов, где доступность является частью абстракции .

Жордау
источник
1
Я полностью согласен с тем, что детали реализации Shoelace не должны загрязнять публичный интерфейс, но я считаю, что этого может быть трудно избежать. То, что вы предлагаете, не всегда будет возможным: цель AutoResetEvent () - общаться между потоками, поэтому его область действия обычно выходит за рамки одного метода.
GrahamS
@GrahamS: вот почему я сказал попробовать спроектировать это таким образом. Вы также можете передать одноразовое использование другим методам. Он выходит из строя только тогда, когда внешние вызовы класса управляют жизненным циклом одноразового использования. Я обновлю ответ соответственно.
Жордау,
2
извините, я знаю, что вы можете раздавать одноразовые вещи, но я все еще не вижу, что это работает. В моем примере AutoResetEventиспользуется для связи между разными потоками, которые работают в одном классе, поэтому он должен быть переменной-членом. Вы не можете ограничить его объем одним методом. (например, представьте, что один поток просто сидит, ожидая некоторой работы, блокируя его waitHandle.WaitOne(). Затем основной поток вызывает shoelace.Tie()метод, который просто выполняет waitHandle.Set()и немедленно возвращается).
GrahamS
3

Это в основном то, что происходит, когда вы смешиваете Composition или Aggregation с Disposable классами. Как уже упоминалось, первым выходом было бы реорганизовать waitHandle из шнурка.

Сказав это, вы можете значительно сократить шаблон Disposable, если у вас нет неуправляемых ресурсов. (Я все еще ищу официальную ссылку для этого.)

Но вы можете опустить деструктор и GC.SuppressFinalize (this); и, возможно, немного очистить виртуальную пустоту Dispose (bool dispose).

Хенк Холтерман
источник
Спасибо, Хенк. Если бы я взял создание waitHandle из Shoelace, тогда кто-то все равно должен был бы где-то избавиться от него, так как этот кто-то узнает, что Shoelace закончил с ним (и что Shoelace не передал его никаким другим классам)?
GrahamS
Вам нужно будет переместить не только создание, но и «ответственность за» ручку ожидания. Ответ jaredPar имеет интересное начало идеи.
Хенк Холтерман,
В этом блоге рассматривается реализация IDisposable и, в частности, рассматривается, как упростить задачу, избегая смешивания управляемых и неуправляемых ресурсов в одном классе. Blog.stephencleary.com/2009/08/…
PaulS
3

Интересно, если Driverопределено, как указано выше:

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

Потом когда Shoeсделано IDisposable, FxCop (v1.36) не жалуется, что Driverтоже должно быть IDisposable.

Однако, если это определено так:

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

тогда он будет жаловаться.

Я подозреваю, что это всего лишь ограничение FxCop, а не решение, потому что в первой версии Shoeэкземпляры все еще создаются, Driverи их нужно как-то утилизировать.

GrahamS
источник
2
Если Show реализует IDisposable, создание его в инициализаторе поля опасно. Интересно, что FXCop не допускает таких инициализаций, поскольку в C # единственный способ сделать их безопасно - это использовать переменные ThreadStatic (совершенно ужасно). В vb.net можно безопасно инициализировать объекты IDisposable без переменных ThreadStatic. Код все еще уродливый, но не такой уж и ужасный. Я бы хотел, чтобы MS предоставила хороший способ безопасно использовать такие инициализаторы полей.
supercat
1
@supercat: спасибо. Вы можете объяснить, почему это опасно? Я предполагаю, что если бы в одном из Shoeконструкторов возникло исключение, то shoesон остался бы в сложном состоянии?
GrahamS
3
Если никто не знает, что от определенного типа IDisposable можно безопасно отказаться, следует убедиться, что каждый созданный объект получает Dispose'd. Если IDisposable создается в инициализаторе поля объекта, но возникает исключение до того, как объект полностью построен, объект и все его поля будут заброшены. Даже если создание объекта заключено в фабричный метод, который использует блок «try» для обнаружения сбоя этой конструкции, фабрике сложно получить ссылки на объекты, требующие удаления.
supercat
3
Единственный известный мне способ справиться с этим в C # - это использовать переменную ThreadStatic для хранения списка объектов, которые потребуют удаления, если текущий конструктор выбрасывает. Затем пусть каждый инициализатор поля регистрирует каждый объект по мере его создания, фабрика очищает список, если он завершается успешно, и использует предложение «finally», чтобы удалить все элементы из списка, если он не был очищен. Это был бы работоспособный подход, но переменные ThreadStatic уродливы. В vb.net можно было использовать подобную технику без каких-либо переменных ThreadStatic.
supercat
3

Я не думаю, что есть технический способ предотвратить распространение IDisposable, если вы сохраните свой дизайн настолько тесно связанным. Тогда следует задаться вопросом, правильный ли дизайн.

В вашем примере, я думаю, имеет смысл, чтобы шнурки были у обуви, и, возможно, водитель должен владеть своей обувью. Однако в автобусе не должен находиться водитель. Как правило, водители автобусов не следуют за автобусами на свалку :) Что касается водителей и обуви, то водители редко делают свою обувь, то есть они на самом деле не «владеют» ими.

Альтернативный дизайн может быть:

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
{
   ...
}

К сожалению, новый дизайн более сложен, поскольку он требует дополнительных классов для создания и удаления конкретных экземпляров обуви и драйверов, но это усложнение является неотъемлемой частью решаемой проблемы. Хорошо то, что автобусы больше не нужно использовать для одноразового использования только для того, чтобы избавиться от шнурков.

Joh
источник
2
Однако это на самом деле не решает исходную проблему. Это может заставить FxCop замолчать, но кое-что все же должно утилизировать шнурки.
AndrewS
Я думаю, что все, что вы сделали, это перенесли проблему в другое место. ShoePairWithDisposableLaces теперь владеет Shoelace (), поэтому его также необходимо сделать IDisposable - так кто избавляется от обуви? Вы оставляете это какому-то контейнеру IoC для обработки?
GrahamS 06
@AndrewS: Действительно, драйверы, обувь и шнурки еще нужно утилизировать, но не автобусы. @GrahamS: Вы правы, я перемещаю проблему в другое место, потому что считаю, что она принадлежит другому. Как правило, существует класс, создающий экземпляр обуви, который также отвечает за утилизацию обуви. Я отредактировал код, чтобы сделать ShoePairWithDisposableLaces одноразовым.
Joh
@Joh: Спасибо. Как вы говорите, проблема с перемещением его в другое место состоит в том, что вы создаете намного больше сложности. Если ShoePairWithDisposableLaces создается каким-то другим классом, тогда разве этот класс не должен быть уведомлен, когда водитель закончит свою обувь, чтобы он мог должным образом Dispose () из них?
GrahamS
1
@ Грэм: да, понадобится какой-то механизм уведомлений. Например, можно использовать политику удаления, основанную на подсчете ссылок. Есть некоторая дополнительная сложность, но, как вы, наверное, знаете, «не существует такой вещи, как бесплатная обувь» :)
Joh
1

Как насчет использования инверсии управления?

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))}));
        }
    }
}
Дарра
источник
Теперь вы сделали это проблемой вызывающих абонентов, и Shoelace не может зависеть от доступности дескриптора ожидания, когда он ему нужен.
Энди
@andy Что вы имеете в виду, говоря «Шнурки не могут зависеть от доступности ручки ожидания, когда она нужна»? Он передается конструктору.
Darragh
Что-то еще могло испортить состояние autoresetevents до передачи его в Shoelace, и оно могло начаться с ARE в плохом состоянии; что-то еще может испортить состояние ARE, пока Shoelace делает свое дело, заставляя Shoelace делать неправильные вещи. По той же причине вы блокируете только частных участников. «В общем, ... избегайте блокировки экземпляров, находящихся вне контроля вашего кода» docs.microsoft.com/en-us/dotnet/csharp/language-reference/…
Энди
Я согласен с блокировкой только частных участников. Но похоже, что ручки ожидания разные. Фактически, кажется более полезным передать их объекту, так как один и тот же экземпляр должен использоваться для связи между потоками. Тем не менее, я думаю, что IoC предоставляет полезное решение проблемы OP.
Darragh
Учитывая, что в примере OPs дескриптор ожидания был закрытым, можно предположить, что объект ожидает монопольный доступ к нему. Отображение дескриптора за пределами экземпляра нарушает это. Обычно IoC может быть полезен для подобных вещей, но не для потоковой передачи.
Энди