Я правильно внедряю IDisposable? - PullRequest
28 голосов
/ 16 июля 2009

Этот класс использует StreamWriter и поэтому реализует IDisposable.

public class Foo : IDisposable
{
    private StreamWriter _Writer;

    public Foo (String path)
    {
        // here happens something along the lines of:
        FileStream fileWrite = File.Open (path, FileMode.OpenOrCreate, FileAccess.Write, FileShare.ReadWrite);
        _Writer = new StreamWriter (fileWrite, new ASCIIEncoding ());
    }

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

    ~Foo()
    {
        Dispose (false);
    }

    protected virtual void Dispose (bool disposing)
    {
        if (_Disposed) {
            return;
        }
        if (disposing) {
            _Writer.Dispose ();
        }
        _Writer = null;
        _Disposed = true;
    }
    private bool _Disposed;
}

}

Есть ли проблемы с текущей реализацией? Т.е. мне нужно вручную высвобождать базовый FileStream? Dispose(bool) написано правильно?

Ответы [ 7 ]

39 голосов
/ 16 июля 2009

Вам не нужно использовать эту расширенную версию реализации IDisposable, если ваш класс напрямую не использует неуправляемые ресурсы.

Простой

 public virtual void Dispose()
 {

     _Writer.Dispose();
 }

будет достаточно.

Если ваш потребитель не удастся утилизировать ваш объект, он будет GC'ом обычно без вызова Dispose, объект, удерживаемый _Writer, также будет GC'd, и у него будет финализатор, поэтому он все равно сможет очистить свой неуправляемый ресурсы правильно.

Редактировать

Проведя некоторые исследования по ссылкам, предоставленным Мэттом и другими, я пришел к выводу, что мой ответ здесь стоит . Вот почему: -

Причиной, стоящей за «шаблоном» одноразовой реализации (под этим я подразумеваю защищенный виртуальный Dispose (bool), SuppressFinalize и т. Д.) Наследуемого класса, является то, что подкласс может держаться за неуправляемый ресурс.

Однако в реальном мире подавляющее большинство из нас .NET-разработчиков никогда не приближаются к неуправляемому ресурсу. Если бы вам пришлось количественно определить « может » выше, какую цифру вероятности вы бы предложили для своего рода .NET-кодирования?

Предположим, у меня есть тип Person (который ради аргумента имеет одноразовый тип в одном из своих полей и, следовательно, должен быть сам по себе одноразовым). Теперь у меня есть наследники Customer, Employee и т. Д. Действительно ли мне разумно загромождать класс Person этим «шаблоном» на тот случай, если кто-то унаследует Person и захочет сохранить неуправляемый ресурс?

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

Если бы мы когда-либо захотели использовать неуправляемый ресурс напрямую, разумный шаблон был бы обернут такую ​​вещь в своем собственном классе, где был бы разумен полный «одноразовый шаблон». Следовательно, в значительно большем объеме «нормального» кода нам не нужно беспокоиться обо всем этом. Если нам нужен IDisposable, мы можем использовать простой шаблон выше, наследуемый или нет.

Фу, рад снять это с моей груди. ;)

16 голосов
/ 16 июля 2009

Вам не нужен метод Finalize (деструктор), поскольку у вас нет неуправляемых объектов. Однако вам следует сохранить вызов GC.SuppressFinalize на случай, если у класса, унаследованного от Foo, есть неуправляемые объекты и, следовательно, финализатор.

Если вы сделаете класс запечатанным, то вы знаете, что неуправляемые объекты никогда не будут входить в уравнение, поэтому нет необходимости добавлять перегрузку protected virtual Dispose(bool) или GC.SuppressFinalize.

Edit:

@ AnthonyWJones возражает против этого: если вы знаете, что подклассы не будут ссылаться на неуправляемые объекты, то все Dispose(bool) и GC.SuppressFinalize не нужны. Но если дело обстоит именно так, вы должны сделать классы internal, а не public, а метод Dispose() должен быть virtual. Если вы знаете, что делаете, то не стесняйтесь следовать предложенному Microsoft шаблону, но вы должны знать и понимать правила, прежде чем нарушать их!

4 голосов
/ 16 июля 2009

Рекомендуется использовать финализатор только в том случае, если у вас есть неуправляемые ресурсы (такие как собственные дескрипторы файлов, указатели памяти и т. Д.).

У меня есть два небольших предложения,

Нет необходимости иметь Переменная "m_Disposed" для проверки Вы ранее звонили Dispose на ваших управляемых ресурсах. Вы могли бы используйте похожий код, например,

protected virtual void Dispose (bool disposing)
{
    if (disposing) {
        if (_Writer != null)
            _Writer.Dispose ();
    }
    _Writer = null;
}

Открывайте файлы только столько времени, сколько необходимо. Итак, в вашем примере вы должны проверить наличие файла в конструкторе, используя File.Exists, а затем, когда вам нужно будет прочитать / записать файл, , затем , вы откроете его и будете использовать. *

Кроме того, если вы просто хотите записать текст в файл, взгляните на File.WriteAllText или File.OpenText или даже File.AppendText, который нацелен на текстовые файлы специально с ASCIIEncoding.

Кроме этого, да, вы правильно реализуете шаблон .NET Dispose.

3 голосов
/ 20 июля 2009

У меня есть много подобных классов, и я рекомендую сделать класс закрытым, когда это возможно. IDisposable + наследование может работать, но в 99% случаев оно вам не нужно - и с ним легко ошибаться.

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

просто сделай:

public sealed class Foo : IDisposable
{
    private StreamWriter _Writer;

    public Foo(string path)
    {
            FileStream fileWrite = File.Open (path, FileMode.OpenOrCreate, FileAccess.Write, FileShare.ReadWrite);
            try { 
                _Writer = new StreamWriter (fileWrite, new ASCIIEncoding());
            } catch {
                fileWrite.Dispose();
                throw;
            }
    }

    public void Dispose()
    {
         _Writer.Dispose();
    }
}

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

Если вы действительно используете много IDisposable с, подумайте над тем, чтобы поместить этот код в C ++ / CLI: это делает весь этот шаблонный код для вас (он прозрачно использует соответствующую технологию детерминированного уничтожения как для собственных, так и для управляемых объектов).

В Википедии есть достойный пример IDisposable для C ++ (действительно, если у вас много IDisposable, C ++ на самом деле намного проще, чем C #): Википедия: C ++ / CLI Финализаторы и автоматические переменные .

Например, реализация «безопасного» одноразового контейнера в C ++ / CLI выглядит следующим образом ...

public ref class MyDisposableContainer
{
    auto_handle<IDisposable> kidObj;
    auto_handle<IDisposable> kidObj2;
public:

    MyDisposableContainer(IDisposable^ a,IDisposable^ b) 
            : kidObj(a), kidObj2(b)
    {
        Console::WriteLine("look ma, no destructor!");
    }
};

Этот код будет корректно располагать kidObj и kidObj2 даже без добавления пользовательской реализации IDisposable, и он устойчив к исключениям в любой реализации Dispose (не то, что это должно произойти, но все же), и его легко поддерживать перед лицом многих других IDisposable участников или родных ресурсов.

Не то, чтобы я был большим поклонником C ++ / CLI, но для сильно ресурсо-ориентированного кода у него легко получается C # beat, и он абсолютно блестяще взаимодействует как с управляемым, так и с нативным кодом - короче говоря, идеальный клейкий код; ). Я склонен писать 90% своего кода на C #, но использую C ++ / CLI для всех потребностей взаимодействия (особенно если вы хотите вызвать любую функцию win32 - MarshalAs и другие атрибуты взаимодействия повсеместно ужасны и совершенно непонятны).

1 голос
/ 16 июля 2009

Вы должны проверить, что _Writer не null, прежде чем пытаться утилизировать его. (Маловероятно, что это когда-нибудь будет null, но на всякий случай!)

protected virtual void Dispose(bool disposing)
{
    if (!_Disposed)
    {
        if (disposing && (_Writer != null))
        {
            _Writer.Dispose();
        }
        _Writer = null;
        _Disposed = true;
    }
}
0 голосов
/ 16 июля 2009

Я согласен со всем, что сказано в других комментариях, но также указал бы на это;

  1. В любом случае вам не нужно устанавливать _Writer = null.

  2. Если вы собираетесь это сделать, то, вероятно, лучше положить его внутрь, если там есть утилизация. Это, вероятно, не имеет большого значения, но вы, как правило, не должны играть с управляемыми объектами, когда они находятся в финализаторе (который в любом случае вам не нужен, как указали другие).

0 голосов
/ 16 июля 2009

Если вы откроете StreamWriter, вы также должны его утилизировать, иначе у вас будет утечка.

...