tags:

views:

537

answers:

3

I have a following class witch uses BinaryReader internally and implements IDisposable.

class DisposableClass : IDisposable
    {
        private BinaryReader reader;
        public DisposableClass(Stream stream)
        {
            reader = new BinaryReader(stream);
        }

        protected virtual void Dispose(bool disposing)
        {
            if (disposing)
            {
                ((IDisposable)reader).Dispose();
//                reader.Dispose();// this won't compile
            }
        }

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

I have already figured out that I need to cast BinaryReader to IDisposable to be able to call Dispose on it, but I don't understand why I can't just call the Dispose() method directly without casting to IDisposable?

+5  A: 

It won't work because the Dispose method on BinaryReader has been explicitly implemented.

Instead of being implicitly implemented, as in:

public void Dispose()
{
}

...it has been explicitly implemented, as in:

void IDisposable.Dispose()
{
}

...which means it can only be accessed via the IDisposable interface. Therefore, you have to cast the instance to IDisposable first.

Jeff Yates
Perfect example of why you shouldn't deviate from the standard Dispose pattern.
Scott Dorman
Or just call BinaryReader.Close() which does the same thing.
Mark Brackett
Even so, if you implement a Close() method, it should do the work normally done in Dispose() and Dispose() should simply call Close(). No matter what, BinaryReader didn't follow the pattern properly.
Scott Dorman
@Scott - true enough. Oddly, the MSDN docs state that Dispose is only to be used internally - which lead me to believe that Close did extra work. But, Reflector shows that Close and Dispose are equivalent. Seems like it could be cleared up in the future without back-compat concerns....
Mark Brackett
@Mark, You are correct, the docs do state "This API supports the .NET Framework infrastructure and is not intended to be used directly from your code." which is even further reason to say it's not following the pattern.
Scott Dorman
There are very few cases where Close and Dispose are not equivalent (mostly in the ADO.NET classes in order to deal with connection pooling). It does seem like something that could be fixed without back-compat concerns, but it's doubtful it will ever change.
Scott Dorman
+3  A: 

Expanding on my comments here, the BinaryReader class does not properly implement the Dispose pattern.

Looking at this class in Reflector, it looks like this (for .NET 3.5):

public class BinaryReader : IDisposable
{
    public virtual void Close()
    {
       this.Dispose(true);
    }
    protected virtual void Dispose(bool disposing)
    {
       if (disposing)
       {
          Stream stream = this.m_stream;
          this.m_stream = null;
          if (stream != null)
          {
             stream.Close();
          }
       }
       this.m_stream = null;
       this.m_buffer = null;
       this.m_decoder = null;
       this.m_charBytes = null;
       this.m_singleChar = null;
       this.m_charBuffer = null;
   }
   void IDisposable.Dispose()
   {
      this.Dispose(true);
   }
}

The problem here is that by making IDisposable.Dispose() an explicit interface implementaiton it forces a developer to call Close() instead of Dispose().

In this context, we have a case of imbalanced semantics. There was never a call to "Open" the reader so it is not intuitive to "Close" the reader.

Going one step further, in order to call Dispose() you must then explicitly cast to IDisposable, which is not something you ordinarily need to do. You do have the option of calling Dispose(bool) directly, but how do you know what the boolean parameter should be?

To properly follow the pattern, it should have been implmented as:

public class BinaryReader : IDisposable
{
    public virtual void Close()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    protected virtual void Dispose(bool disposing)
    {
       if (disposing)
       {
          Stream stream = this.m_stream;
          this.m_stream = null;
          if (stream != null)
          {
             stream.Close();
          }
       }
       this.m_stream = null;
       this.m_buffer = null;
       this.m_decoder = null;
       this.m_charBytes = null;
       this.m_singleChar = null;
       this.m_charBuffer = null;
   }
   public void Dispose()
   {
      this.Close();
   }
}

This would allow you to call either Close() or Dispose(), in which case either call continues to result in calling Dispose(true). (This is the same flow as the actual implementation by calling Close() or ((IDisposable)reader).Dispose()).

Fortunately (or unfortunately, depending on which way you choose to look at it) because BinaryReader does implement the IDisposable interface it is allowed in a using statement:

using (BinaryReader reader = new BinaryReader(...))
{
}
Scott Dorman
"You do have the option of calling Dispose(bool) directly" - it's protected, so that's not an option.
Mark Brackett
@Mark, your comment contradicts itself...Dispose(bool) is protected so it's not directly accessible unless you inherit from BinaryReader, which might be an option in this case depending on the usage.
Scott Dorman
A: 

Actually they have chosen to use Close() instead of Dispose() Dispose has been explicitly implemented. Which is why you can't see it.

However Close does the same thing as dispose and this is the method they want you to use. Reflector gives the following disassembly for the Close method

public virtual void Close()
{
    this.Dispose(true);
}

Close() is used because it is a better choice of words in the context of a binary reader.

trampster
Any ideas why they went down this path?
Damien