About IDisposable, Close(), Streams and StreamReader-Writer

Last week, we discovered a bug in a SOAP extension our team wrote. An ObjectDisposedException occurred during the extension invocation on the client side.

The call stack ended like this:

System.ObjectDisposedException: Cannot access a closed Stream.

at System.IO.__Error.StreamIsClosed()
at System.IO.MemoryStream.set_Position(Int64 value)
…(upper in call chain)

Oops, we're trying to use a closed Stream…How can that be? Well, the code that actually uses the Stream is the following:

public void ProcessMessage()
{
    // ...
    CopyStream(_oldStream, _newStream);
    _newStream.Position = 0;// <= this is where the exception occurs
    // ...
}

private static void CopyStream(Stream source, Stream target)
{
    using (TextReader reader = new StreamReader(source))
    using (TextWriter writer = new StreamWriter(target))
    {
        writer.Write(reader.ReadToEnd());
    }
}

It looks like _newStream is being closed inside CopyStream, and when I look at the method, the only place where this could happen is the Dispose call on the StreamWriter, when leaving the using block. So, it looks like the StreamWriter is effectively becoming the owner of the underlying Stream it uses! Let's look at what really happens in StreamWriter.Dispose, thanks to our friend Reflector.

public abstract class TextWriter : MarshalByRefObject, IDisposable
{
    public virtual void Close()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

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

    protected virtual void Dispose(bool disposing)
    {
    }

    // Other members...
}

public class StreamWriter : TextWriter
{
    private Stream stream;
    private bool closable;

    internal bool Closable
    {
        get
        {
            return this.closable;
        }
    }

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

    protected override void Dispose(bool disposing)
    {
        try
        {
            // some code omitted here...
        }
        finally
        {
            if (this.Closable && (this.stream != null))
            {
                try
                {
                    if (disposing)
                    {
                        this.stream.Close();
                    }
                }
                finally
                {
                    this.stream = null;
                    // setting other members to null here…
                    base.Dispose(disposing);
                }
            }
        }
    }
}

Ah aah, OK, I understand. Indeed when disposing of the StreamWriter, it calls Stream.Close. But I noticed that there is this Closable property, that when set to true, could avoid the closing of the Stream. Unfortunately, it's internal, and it happens to be used by the framework only (for Console streams apparently). Frankly, I don't like this design. I think it would have been better to make Stream ownership by the StreamWriter optional, and that's what the Closable property seems to have as purpose, so why isn't there a way to set it? The only StreamWriter constructor that allows setting it is also internal. Moreover, why does TextWriter.Dispose call GC.SuppressFinalize(this) while there is no finalizer defined in the class? And there is also the override of the Close method which just…repeats the base implementation (I'll discuss this implementation further, together with Stream).

Hmm, I'm not happy with what I discovered in the framework, but maybe I would not have had this bug if I had read the doc for the StreamWriter constructor? So I read it, and guess what? It is nowhere mentioned that the lifetime of my Stream object is now tightly coupled to the StreamWriter. Not a single word about it. But there's a nice sample of code illustrating the usage of the different constructors of StreamWriter. Here it is:

public void CreateTextFile(string fileName, string textToAdd)
{
    string logFile = DateTime.Now.ToShortDateString()
        .Replace(@"/", @"-").Replace(@"\", @"-") + ".log";

    FileStream fs = new FileStream(fileName,
        FileMode.CreateNew, FileAccess.Write, FileShare.None);

    StreamWriter swFromFile = new StreamWriter(logFile);
    swFromFile.Write(textToAdd);
    swFromFile.Flush();
    swFromFile.Close();

    StreamWriter swFromFileStream = new StreamWriter(fs);    // <= look at this
    swFromFileStream.Write(textToAdd);
    swFromFileStream.Flush();
    swFromFileStream.Close();    // <= and also at this…

    StreamWriter swFromFileStreamDefaultEnc =
        new System.IO.StreamWriter(fs,    // <= and finally at this…
        System.Text.Encoding.Default);
    swFromFileStreamDefaultEnc.Write(textToAdd);
    swFromFileStreamDefaultEnc.Flush();
    swFromFileStreamDefaultEnc.Close();

    // more code omitted…
}

If you try to run the sample, it will of course miserably fail…Because closing the StreamWriter also closes the underlying Stream, so it can't be used again on the next line.

But that's not all…What I also found a little bit strange when looking at the above code, is the fact that it's Stream.Close that's being called by StreamWriter.Dispose, and not Stream.Dispose. Why? Shouldn't owned IDisposable objects be Disposed of during Dispose? Well, it happens that for Stream-derived classes, calling Dispose or Close has strictly the same effect. Let's look at the code:

public abstract class Stream : MarshalByRefObject, IDisposable
{
    public void Dispose()
    {
        this.Close();
    }

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

    protected virtual void Dispose(bool disposing)
    {
        if (disposing && (this._asyncActiveEvent != null))
        {
            this._CloseAsyncActiveEvent(Interlocked.Decrement(ref this._asyncActiveCount));
        }
    }

    // Other members...
}

public class MemoryStream : Stream
{
    protected override void Dispose(bool disposing)
    {
        try
        {
            if (disposing)
            {
                this._isOpen = false;
                this._writable = false;
                this._expandable = false;
            }
        }
        finally
        {
            base.Dispose(disposing);
        }
    }

    // Other members...
}

So Dispose calls Close, which calls Dispose(true) in the Stream base class, and MemoryStream.Dispose(bool) does nothing interesting but calling the base implementation. Wow, what's that? Again, I don't like that design. Why have a Dispose and a Close when both are doing the same? IMO, I think something is flawed in Stream (and StreamWriter/Reader…). There's a semantic difference between disposing of an object and closing it: in the former case, I don't need it anymore and it's gone, while in the latter, I might to re-open it and use it again. I'm thinking for example of a magazine: when I read it in its entirety, I put it in the recycle bin (that's the Dispose), and when I just read some articles and I plan to read it further later, I close it and leave it on my desk.

To Close or to Dispose?

I think having Close call Dispose brings some confusion. Why have the two? Anyway the contract for a user of a class that implements IDisposable is that it is mandatory to call Dispose after use. Any call to Close is then superfluous. If Close would have allowed me to temporarily release some resource and reuse it later through a call to some Open method, then I think it would make sense. There's another place in the framework 2.0 where this Dispose(),
Dispose(bool), Finalize(), Open(), Close() stuff is IMO almost correctly implemented: System.Data.SqlClient.SqlConnection. I say almost, because trying to re-Open a disposed connection yields an InvalidOperationException and not an ObjectDisposedException, as I would expect. For the rest, it is quite possible to call Open and Close multiple times without any problem.

A simple solution to the Stream ownership issue

The solution I found to the above issue (I'm actually talking about the Stream being closed by the StreamWriter) is rather simple (and I guess I'm not the first one to implement it): use a Decorator around the Stream to prevent it from being closed or disposed. That way, the Stream object is still usable even after the StreamWriter/Reader has been closed.

public class NonClosingStreamDecorator : Stream
{
    private readonly Stream _innerStream;

    public NonClosingStreamDecorator(Stream stream)
    {
        if (stream == null)
            throw new ArgumentNullException("stream");
        _innerStream = stream;
    }

    public override void Close()
    {
        // do not delegate !!
        //_innerStream.Close();
    }

    protected override void Dispose(bool disposing)
    {
        // do not delegate !!
        //_innerStream.Dispose();
    }

    // The rest of the overriden methods simply delegate to InnerStream
}

Using the Decorator is straightforward:

[Test]
public void TestClosingStreamWriterWithNonClosingStream()
{
    TextWriter writer = new StreamWriter(new NonClosingStreamDecorator(_stream));
    writer.WriteLine("I like this");
    writer.Close();
    // enjoy using the Stream further
    _stream.Position = 0;
}