Am I implementing IDisposable correctly?

Question

This class uses a StreamWriter and therefore implements 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;
}

}

Is there any issue with the current implementation? I.e., do I have to release the underlying FileStream manually? Is Dispose(bool) written correctly?

Answer 1

I have many classes like this - and my recommendation is to make the class sealed whenever possible. IDisposable + inheritance can work, but 99% of the times you don't need it - and it's easy to make mistakes with.

Unless you're writing a public API (in which case it's nice to permit people to implement your code however they wish to - i.e. to use IDisposable), you don't need to support it.

just do:

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

Note the try...catch; technically the StreamWriter constructor could throw an exception, in which case it never takes ownership of the FileStream and you must dispose it yourself.

If you're really using many IDisposables, consider putting that code in C++/CLI: that makes all that boilerplate code for you (it transparently uses the appropriate deterministic destruction tech for both native and managed objects).

Wikipedia has a decent IDisposable sample for C++ (really, if you have many IDisposable's, C++ is actually much simpler than C#): Wikipedia: C++/CLI Finalizers and automatic variables.

For example, implementing a "safe" disposable container in C++/CLI looks like...

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

This code will correctly dispose kidObj and kidObj2 even without adding a custom IDisposable implementation, and it's robust to exceptions in either Dispose implementation (not that those should occur, but still), and simple to maintain in the face of many more IDisposable members or native resources.

Not that I'm a huge fan of C++/CLI, but for heavily resource-oriented code it's got C# beat easily, and it has absolutely brilliant interop both with managed and native code - in short, perfect glue code ;-). I tend to write 90% of my code in C#, but use C++/CLI for all interop needs (esp. if you want to call any win32 function - MarshalAs and other interop attributes all over the place are horrifying and thoroughly incomprehensible).

Answer 2

You don't need to use this extensive version of IDisposable implementation if your class doesn't directly use unmanaged resources.

A simple

 public virtual void Dispose()
 {

     _Writer.Dispose();
 }

will suffice.

If your consumer fails to Dispose your object it will be GC'd normally without a call to Dispose, the object held by _Writer will also be GC'd and it will have a finaliser so it still gets to clean up its unmanaged resources properly.

Edit

Having done some research on the links provided by Matt and others I've come to the conclusion that my answer here stands. Here is why:-

The premise behind the disposable implementation "pattern" (by that I mean the protected virtual Dispose(bool), SuppressFinalize etc. marlarky) on an inheritable class is that a sub-class might hold on to an unmanaged resource.

However in the real world the vast majority of us .NET developers never go anywhere near an unmanaged resource. If you had to quantify the "might" above what probabilty figure would you come up with for you sort of .NET coding?

Lets assume I have a Person type (which for sake of argument has a disposable type in one of its fields and hence ought to be disposable itself). Now I have inheritors Customer, Employee etc. Is it really reasonable for me to clutter the Person class with this "Pattern" just in case someone inherits Person and wants to hold an unmanaged resource?

Sometimes we developers can over complicate things in an attempt to code for all possible circumstances without using some common sense regarding the relative probability of such circumstances.

If we ever wanted to use an unmanaged resource directly the sensible pattern would be wrap such a thing in its own class where the full "disposable pattern" would be reasonable. Hence in the significantly large body of "normal" code we do not to have to worry about all that mucking about. If we need IDisposable we can use the simple pattern above, inheritable or not.

Phew, glad to get that off my chest. ;)

Answer 3

You don't need a Finalize (destructor) method since you don't have any unmanaged objects. However, you should keep the call to GC.SuppressFinalize in case a class inheriting from Foo has unmanaged objects, and therefore a finalizer.

If you make the class sealed, then you know that unmanaged objects are never going to enter the equation, so it isn't necessary to add the protected virtual Dispose(bool) overload, or the GC.SuppressFinalize.

Edit:

@AnthonyWJones's objection to this is that if you know that the subclasses will not reference unmanaged objects, the whole Dispose(bool) and GC.SuppressFinalize is unnecessary. But if this is the case you should really make the classes internal rather than public, and the Dispose() method should be virtual. If you know what you are doing, then feel free not to follow Microsoft's suggested pattern, but you should know and understand the rules before you break them!

Answer 4

I agree with everything said in the other comments, but would also point out that;

1. You don't actually need to set _Writer = null in either case.

2. If you're going to do so, it's probably best to put it inside the if where the dispose is. It probably doesn't make a big differece, but you're generally not supposed to play with managed objects when being disposed by the finaliser (which you don't need in this case anyway, as others have pointed out).

Answer 5

You should check that _Writer isn't null before attempting to dispose it. (It doesn't seem likely that it ever will be null, but just in case!)

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

Answer 6

The recommended practice is to use a finalizer only when you have unmanaged resources (such as native file handles, memory pointers etc).

I have two small suggestions though,

It's not necessary to have the "m_Disposed" variable to test if you have previously called Dispose on your managed resources. You could use similar code like so,

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

Open files for only as long as is necessary. So in your example, you would test for the existence of the file in the constructor using File.Exists and then when you need to read/write the file, then you would open it and use it.

Also, if you merely weant to write text to a file, have a look at File.WriteAllText or File.OpenText or even File.AppendText which is aimed at text files specifically with ASCIIEncoding.

Other than that, yes you are implementing the .NET Dispose pattern correctly.

Answer 7

If you open the StreamWriter, you have to Dispose it as well, or you'll have a leak.