196

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:

Is it good?

Stas Ivanov
  • 1,173
  • 1
  • 14
  • 24
Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • 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. – Thorsten Hans Sep 23 '11 at 06:11
  • 3
    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 } – Latrova Sep 22 '14 at 13:49
  • possible duplicate of [Does Stream.Dispose always call Stream.Close (and Stream.Flush)](http://stackoverflow.com/questions/911408/does-stream-dispose-always-call-stream-close-and-stream-flush) – Frédéric Sep 25 '15 at 08:18
  • 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": http://stackoverflow.com/questions/5282999/reading-csv-file-and-storing-values-into-an-array/5283044#5283044 – Timothy Gonzalez Jan 31 '17 at 21:27
  • 1
    @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. – Suncat2000 Oct 12 '17 at 14:46
  • 2
    @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")) { }` – Timothy Gonzalez Oct 12 '17 at 15:17
  • 1
    @TimothyGonzalez Sorry to be pedantic but those latter statements _are_ nested. – Suncat2000 Oct 18 '17 at 13:13

7 Answers7

142

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.

Enigmativity
  • 113,464
  • 11
  • 89
  • 172
  • 26
    "*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. – Heinzi Sep 23 '11 at 06:38
  • @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. – Enigmativity Sep 23 '11 at 06:49
  • 128
    Hmm, no, that is a "why the heck is he closing it twice??" speed bump while reading. – Hans Passant Sep 23 '11 at 08:14
  • @Enigmativity: Good point. (+1 for the good and detailed answer, BTW) – Heinzi Sep 23 '11 at 08:58
  • @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". :-) – Enigmativity Sep 23 '11 at 10:58
  • 72
    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. – R. Martinho Fernandes Nov 14 '11 at 23:58
  • @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. – Enigmativity Nov 15 '11 at 00:09
  • 21
    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. – marc40000 Feb 15 '12 at 09:39
  • @marc40000 - Interesting info. Much appreciated. – Enigmativity Feb 15 '12 at 10:13
  • 4
    +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. – Francis Rodgers Dec 12 '12 at 16:34
  • 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. – ThunderGr Dec 31 '13 at 09:30
  • @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. – Enigmativity Dec 31 '13 at 10:26
  • 2
    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? – Dunk Jun 25 '14 at 12:38
  • @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. – Enigmativity Jun 25 '14 at 23:58
  • 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. – Dunk Jun 26 '14 at 14:45
  • @Dunk - I agree. Cheers. – Enigmativity Jun 26 '14 at 23:33
  • 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](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.close) states *"Instead of calling this method, ensure that the stream is properly disposed."* – Herohtar Dec 29 '19 at 23:15
  • Using doesn't help if the Stream isn't confined to a local scope. The below answers correctly say that .Dispose should be used as .Close is an outdated method that was created before IDisposable. – eriyg Dec 21 '22 at 17:55
  • @marc40000: Calling Dispose multiple times is explicitly permitted. That code analysis is incorrect (you can't get `ObjectDisposedException` from `Dispose()`, not from any class following the `IDisposable` contract). Not saying it shouldn't be flagged at all -- the programmer probably didn't intend multiple dispose, after all -- but the instruction that comes with it is 100% incorrect. https://learn.microsoft.com/en-us/dotnet/api/system.idisposable.dispose – Ben Voigt Feb 23 '23 at 22:41
  • "If an object's `Dispose` method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its `Dispose` method is called multiple times. Instance methods other than `Dispose` can throw an `ObjectDisposedException` when resources are already disposed." – Ben Voigt Feb 23 '23 at 22:42
58

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().

Yousha Aleayoub
  • 4,532
  • 4
  • 53
  • 64
Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • 1
    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 – Isak Savo Sep 23 '11 at 06:41
  • 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? – Nawaz Sep 23 '11 at 06:43
  • @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. – Darin Dimitrov Sep 23 '11 at 06:46
  • 7
    Terrible answer. It assumes you can use a `using` block. I'm implementing a class that writes from time to time and therefore cannot. – Jez Dec 06 '14 at 15:08
  • 8
    @Jez Your class should then implement the IDisposable interface, and possibly also Close() [if close is standard terminology in the area](https://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx), so that classes using your class can use `using` (or, again, go for the Dispose Pattern). – Dorus Jun 09 '15 at 14:14
  • 1
    The OP asked about properly closing stream objects. Not about some syntactic sugar. – GuardianX Feb 17 '21 at 23:26
  • using doesn't help if the Stream isn't confined to a local scope. The below answers correctly say that .Dispose should be used as .Close is an outdated method that was created before IDisposable. – eriyg Dec 21 '22 at 17:56
19

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".

Heinzi
  • 167,459
  • 57
  • 363
  • 519
15

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.

Joe Amenta
  • 4,662
  • 2
  • 29
  • 38
14

This is an old question, but you can now(C# 8.0) 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://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using

Yousha Aleayoub
  • 4,532
  • 4
  • 53
  • 64
Todd Skelton
  • 6,839
  • 3
  • 36
  • 48
  • Wow. Thanks for the warning! Talk about confusing. It was enough of an exercise looking up which block-end brace disposed the stream. Now we can guess if it can remain open until the end of the function? What were they thinking? Microsoft developers: creating sharp edges since the 20th century! – Suncat2000 Aug 10 '23 at 21:45
7

On many classes which support both Close() and Dispose() methods, the two calls would be equivalent. On some classes, however, it is possible to re-open an object which has been closed. Some such classes may keep some resources alive after a Close, in order to permit reopening; others may not keep any resources alive on Close(), but might set a flag on Dispose() to explicitly forbid re-opening.

The contract for IDisposable.Dispose explicitly requires that calling it on an object which will never be used again will be at worst harmless, so I would recommend calling either IDisposable.Dispose or a method called Dispose() on every IDisposable object, whether or not one also calls Close().

Yousha Aleayoub
  • 4,532
  • 4
  • 53
  • 64
supercat
  • 77,689
  • 9
  • 166
  • 211
  • FYI here's an article on the MSDN blogs that explains the Close and Dispose fun. http://blogs.msdn.com/b/kimhamil/archive/2008/03/15/the-often-non-difference-between-close-and-dispose.aspx – JamieSee Apr 16 '15 at 23:17
-1

Just to complement other answers, as of C# 8.0, you don't need to open a block of code just to use an using statement

if (...) 
{ 
   using FileStream f = new FileStream(@"C:\users\jaredpar\using.md");
   // statements
}

// Equivalent to 
if (...) 
{ 
   using (FileStream f = new FileStream(@"C:\users\jaredpar\using.md")) 
   {
    // statements
   }
}

docs:

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

Rafael Kuhn
  • 67
  • 1
  • 3