Should I call Close() or Dispose() for stream objects?

Question

Classes such as Stream, StreamReader, StreamWriter etc implements IDisposable interface. That means, we can call Dispose() method on objects of these classes. They've also defined a public method called Close(). Now that confuses me, as to what should I call once I'm done with objects? What if I call both?

My current code is this:

using (Stream responseStream = response.GetResponseStream())
{
   using (StreamReader reader = new StreamReader(responseStream))
   {
      using (StreamWriter writer = new StreamWriter(filename))
      {
         int chunkSize = 1024;
         while (!reader.EndOfStream)
         {
            char[] buffer = new char[chunkSize];
            int count = reader.Read(buffer, 0, chunkSize);
            if (count != 0)
            {
               writer.Write(buffer, 0, count);
            }
         }
         writer.Close();
      }
      reader.Close();
   }
}

As you see, I've written using() constructs, which automatically call Dispose() method on each object. But I also call Close() methods. Is it right?

Please suggest me the best practices when using stream objects. :-)

MSDN example doesn't use using() constructs, and call Close() method:

• How to: Download Files with FTP

Is it good?

Solution 1

A quick jump into Reflector.NET shows that the Close() method on StreamWriter is:

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

And StreamReader is:

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

The Dispose(bool disposing) override in StreamReader is:

protected override void Dispose(bool disposing)
{
    try
    {
        if ((this.Closable && disposing) && (this.stream != null))
        {
            this.stream.Close();
        }
    }
    finally
    {
        if (this.Closable && (this.stream != null))
        {
            this.stream = null;
            /* deleted for brevity */
            base.Dispose(disposing);
        }
    }
}

The StreamWriter method is similar.

So, reading the code it is clear that that you can call Close() & Dispose() on streams as often as you like and in any order. It won't change the behaviour in any way.

So it comes down to whether or not it is more readable to use Dispose(), Close() and/or using ( ... ) { ... }.

My personal preference is that using ( ... ) { ... } should always be used when possible as it helps you to "not run with scissors".

But, while this helps correctness, it does reduce readability. In C# we already have plethora of closing curly braces so how do we know which one actually performs the close on the stream?

So I think it is best to do this:

using (var stream = ...)
{
    /* code */

    stream.Close();
}

It doesn't affect the behaviour of the code, but it does aid readability.

Solution 2

No, you shouldn't call those methods manually. At the end of the using block the Dispose() method is automatically called which will take care to free unmanaged resources (at least for standard .NET BCL classes such as streams, readers/writers, ...). So you could also write your code like this:

using (Stream responseStream = response.GetResponseStream())
    using (StreamReader reader = new StreamReader(responseStream))
        using (StreamWriter writer = new StreamWriter(filename))
        {
            int chunkSize = 1024;
            while (!reader.EndOfStream)
            {
                 char[] buffer = new char[chunkSize];
                 int count = reader.Read(buffer, 0, chunkSize);
                 if (count != 0)
                 {
                     writer.Write(buffer, 0, count);
                 }
            }
         }

The Close() method calls Dispose().

Solution 3

The documentation says that these two methods are equivalent:

StreamReader.Close: This implementation of Close calls the Dispose method passing a true value.

StreamWriter.Close: This implementation of Close calls the Dispose method passing a true value.

Stream.Close: This method calls Dispose, specifying true to release all resources.

So, both of these are equally valid:

/* Option 1, implicitly calling Dispose */
using (StreamWriter writer = new StreamWriter(filename)) { 
   // do something
} 

/* Option 2, explicitly calling Close */
StreamWriter writer = new StreamWriter(filename)
try {
    // do something
}
finally {
    writer.Close();
}

Personally, I would stick with the first option, since it contains less "noise".

Solution 4

This is an old question, but you can now write using statements without needing to block each one. They will be disposed of in reverse order when the containing block is finished.

using var responseStream = response.GetResponseStream();
using var reader = new StreamReader(responseStream);
using var writer = new StreamWriter(filename);

int chunkSize = 1024;
while (!reader.EndOfStream)
{
    char[] buffer = new char[chunkSize];
    int count = reader.Read(buffer, 0, chunkSize);
    if (count != 0)
    {
        writer.Write(buffer, 0, count);
    }
}

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using

Solution 5

For what it's worth, the source code for Stream.Close explains why there are two methods:

// Stream used to require that all cleanup logic went into Close(),
// which was thought up before we invented IDisposable.  However, we
// need to follow the IDisposable pattern so that users can write
// sensible subclasses without needing to inspect all their base
// classes, and without worrying about version brittleness, from a
// base class switching to the Dispose pattern.  We're moving
// Stream to the Dispose(bool) pattern - that's where all subclasses
// should put their cleanup now.

In short, Close is only there because it predates Dispose, and it can't be deleted for compatibility reasons.

View more solutions

Comments

[Thorsten Hans]

If yo're using ReSharper you could define this as a "antipattern" within the patter catalog. ReSharper will mark each usage as error/hint/warning regarding to your definition. It's also possible to define how ReSharper has to apply a QuickFix for such an occurrence.

[Latrova]

Just a tip: You can use the using statement like that for multiple disposable itens: using (Stream responseStream = response.GetResponseStream()) using (StreamReader reader = new StreamReader(responseStream)) using (StreamWriter writer = new StreamWriter(filename)) { //...Some code }

[Frédéric]

possible duplicate of Does Stream.Dispose always call Stream.Close (and Stream.Flush)

[Timothy Gonzalez]

You don't need to nest your using statements like that you can stack them on top of one another and have one set of brackets. On another post, I suggested an edit for a code snippet that should have had using statements with that technique if you'd like to look and fix your "code arrow": stackoverflow.com/questions/5282999/…

[Suncat2000]

@TimothyGonzalez You do have to nest your using statements like that. using permits only one type, even if you are initializing multiple resources in the same statement. If you are using multiple statements or multiple types, by definition you must nest using statements; here, the objects are different types and must be in separate using statements.

[Timothy Gonzalez]

@Suncat2000 You can have multiple using statements, but not nest them and instead stack them. I don't mean syntax like this which restricts the type: using (MemoryStream ms1 = new MemoryStream(), ms2 = new MemoryStream()) { }. I mean like this where you can redefine the type: using (MemoryStream ms = new MemoryStream()) using (FileStream fs = File.OpenRead("c:\\file.txt")) { }

[Suncat2000]

@TimothyGonzalez Sorry to be pedantic but those latter statements are nested.

[Can Sahin]

"In C# we already have plethora of closing curly braces so how do we know which one actually performs the close on the stream?" I don't think that this is a big problem: The stream is closed "at the right time", i.e., when the variable goes out of scope and is no longer needed.

[Isak Savo]

I'm pretty sure you don't need to be using the first responseStream since that is wrapped by the reader which will make sure its closed when the reader is disposed. +1 nontheless

[Nawaz]

This is confusing when you said The Close method calls Dispose... and in the rest of your post, you're implying that Dispose() would call Close(), I shouldn't call the latter manually. Are you saying they call each other?

[Darin Dimitrov]

@Nawaz, my post was confusing. The Close method simply calls Dispose. In your case you need Dispose in order to free unmanaged resources. By wrapping your code in using statement the Dispose method is called.

[Enigmativity]

@Heinzi - I agree that the stream is closed at the right time. My position is that readability of the code is improved by including the Close and including the Close is benign as it doesn't change the behaviour.

[user1703401]

Hmm, no, that is a "why the heck is he closing it twice??" speed bump while reading.

[Can Sahin]

@Enigmativity: Good point. (+1 for the good and detailed answer, BTW)

[Enigmativity]

@HansPassant - I'd look at it from the other POV. You're obviously an experienced developer so you know that using effectively closes the stream so it seems like I'm closing it twice - but again you're experienced so you know that it doesn't matter if I do. However, if someone less experienced looks at the code they might think it wasn't closed unless Close was explicitly called. This pattern makes coding safer for inexperienced developers and just gives experienced developers a "chuckle". :-)

[R. Martinho Fernandes]

I disagree with the redundant Close() call. If someone less experienced looks at the code and doesn't know about using he will: 1) look it up and learn, or 2) blindly add a Close() manually. If he picks 2), maybe some other developer will see the redundant Close() and instead of "chuckling", instruct the less experienced developer. I'm not in favor of making life difficult for inexperienced developers, but I'm in favor of turning them into experienced developers.

[Enigmativity]

@R.MartinhoFernandes - I'm an experienced developer and I would instruct less experienced developers to include the redundant close for the reasons I have given. I do think this discussion is boiling down to a simple "matter of opinion" rather than what is correct/best or not. It's good that opinions have been aired though. Cheers.

[marc40000]

If you use using + Close() and turn /analyze on, you get "warning : CA2202 : Microsoft.Usage : Object 'f' can be disposed more than once in method 'Foo(string)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object.: Lines: 41" So while the current implementation is fine with calling Close and Dispose, according to documentation and /analyze, it's not ok and might change in future versions of .net.

[Enigmativity]

@marc40000 - Interesting info. Much appreciated.

[Francis Rodgers]

+1 for the good answer. Another thing to consider. Why not add a comment after the closing brace like //Close or as I do, being a newbie, I add a one liner after any closing brace thats not clear. like for example in a long class I would add //End Namespace XXX after the final closing brace, and //End Class YYY after the second final closing brace. Is this not what comments are for. Just curious. :) As a newbie, I saw such code, hense why I came here. I did ask the question "Why the need for the second close". I feel extra lines of code dont add to clarity. Sorry.

[ThunderGr]

I like my code to say what it is doing. So, I'd rather use the .Dispose() explicitly.Using is optional anyway and, IMHO, it may or may not be convenient to use, according to circumstances and personal preference. Using it, adds one more block to your code which is unneeded if you explicitly dispose your objects.

[Enigmativity]

@ThunderGr - I'd rather the extra block than forgetting to cal Dispose. I think the using keyword is explicit enough. Also using automatically adds in a try/catch that ensures that the dispose is called. It actually changes the semantics for the better, I think.

[akash doijode]

While I appreciate the details of the answer, I really don't like relying on implementation details of 3rd party functionality for making a decision on the "proper" way to do things. If Microsoft decides to change the implementation so it does matter whether you call Close or Dispose (and in the right order) then whose fault is it when your application breaks because you are relying on implementation details and not documentation details?

[Enigmativity]

@Dunk - I don't think my suggested coding style would be affected by a change from Microsoft. I don't think that they'd change this behaviour now in any case.

[akash doijode]

I wasn't specifically criticizing your example. I really liked your answer, it was informative. I actually think Microsoft wrote it this way to avoid confusion, so I agree they won't change it. However, I was more intending to make a general comment that relying on a current 3rd party implementation is a dangerous practice. It can change at any time and finding the cause could be really, really hard.

[Enigmativity]

@Dunk - I agree. Cheers.

[Jez]

Terrible answer. It assumes you can use a using block. I'm implementing a class that writes from time to time and therefore cannot.

[JamieSee]

FYI here's an article on the MSDN blogs that explains the Close and Dispose fun. blogs.msdn.com/b/kimhamil/archive/2008/03/15/…

[Dorus]

@Jez Your class should then implement the IDisposable interface, and possibly also Close() if close is standard terminology in the area, so that classes using your class can use using (or, again, go for the Dispose Pattern).

[Herohtar]

Calling Close() inside a using block clutters your code and makes it confusing for everyone -- either they'll think the extra close is necessary or they'll be wondering why in the world someone added a redundant line. The documentation itself states "Instead of calling this method, ensure that the stream is properly disposed."

[GuardianX]

The OP asked about properly closing stream objects. Not about some syntactic sugar.