Using statement in context of streams and WebClients

Question

Mostly a question on which approach would be better and when utilizing the using keyword, would it still be neccesary to close streams programatically. If an object implements IDisposable should using always be used?

Link to my full answer using this sample: Stackoverflow Answer

Option 1:

WebClient webClient;
webClient = new WebClient();
string json = webClient.DownloadString(@"urlToTheJsonResponse");
MemoryStream stream = new MemoryStream((Encoding.UTF8.GetBytes(json)));
DataContractJsonSerializer ser = new DataContractJsonSerializer(typeof(GISData));
stream.Position = 0;
GISData data = (GISData)ser.ReadObject(stream);
stream.Close();

Option 2:

WebClient webClient;
MemoryStream stream;
string json;
GISData data;
using (webClient = new WebClient())
{
 json = webClient.DownloadString(@"urlToTheJsonResponse");
}
using (stream = new MemoryStream((Encoding.UTF8.GetBytes(json))))
{
 DataContractJsonSerializer ser = new DataContractJsonSerializer(typeof(GISData));
 data = (GISData)ser.ReadObject(stream);
 stream.Close();
}
using (webClient = new WebClient())
{
 webClient.DownloadFile(data.href, "C:/" + data.href.Substring(data.href.LastIndexOf("/") + 1));
}

Answer 1

Under normal circumstances, you should always either call Dispose() explicitly or use a using block. This applies especially for streams and related types (like StreamWriter), where not disposing can have very visible bad consequences (e.g. the end of the text won't be written to the file).

But there are some types for which calling Dispose() doesn't actually do anything useful. And WebClient and MemoryStream are among them. They are IDisposable primarily because they inherit that interface from their base class (Component and Stream, respectively). So, I think your Option 1 is fine.

If you are unsure whether you should dispose objects of some type or whether it's one of the few types where not disposing is safe, err on the side of caution and dispose it.

Answer 2

I realize this is over two years later, but I thought I should point out a redundancy in the code in the OP. I see this:

using (stream = new MemoryStream((Encoding.UTF8.GetBytes(json)))) {
 DataContractJsonSerializer ser = new DataContractJsonSerializer(typeof(GISData));
 data = (GISData)ser.ReadObject(stream);
 stream.Close();
}

Notice that stream.Close(); at the end of the block - which, of course, is unnecessary since stream is the object of the using.

(I would have just used comment under the OP, but I just signed up to this particular branch of stackexchange and don't have the rep requirement.)

• holroy: Welcome to Code Review SE. Good catch on the stream.Close().

Answer 3

Yes you should, classes which implement IDisposable are basically stating that they internally have used managed or unmanaged resources which require explicit tidy up outside of the remit of the garbage collector. The using statement is simply a compiler short cut to a try/finally block which ensures that the dispose method is called even if the code inside the using block throws an exception.

using (webClient = new WebClient())
{
 webClient.DownloadFile(data.href, "C:/" + data.href.Substring(data.href.LastIndexOf("/") + 1));
}

becomes

WebClient webClient = new WebClient();
try
{
 webClient.DownloadFile(data.href, "C:/" + data.href.Substring(data.href.LastIndexOf("/") + 1));
}
finally
{
 if (webClient != null)
 {
 webClient.Dispose();
 }
}

Although the garbage collector will call the Finalize() method on any IDisposable instance that implements one, that won't happen until the object is available for garbage collection and therefore the tidy up won't happen until some point in the future.

Most classes with a finalizer will call the dispose method but usually only tidy up unmanaged resources when called from the finalizer, therefore it is still recommended to call dispose yourself as soon as you are finished with an object to ensure that you don't hold on to other resources longer than necessary.

• svick: This is completely wrong. The GC never calls Dispose(). But it does call the finalizer, which usually does the same cleanup of unmanaged resources as Dispose().
• Sean H: @svick - I don't think you understand the meaning of the word "completely." For example, finding a pedantic error among a multi-faceted answer that is otherwise correct is not "completely wrong." I'm only saying something because your comment may mislead some people with regards to the quality of the question.
• svick: @SeanH, well, I don't think that's a pedantic error. I think it's quite important and if you believe what Trevor wrote, it might bite you big time.
• Trevor Pilley: @svick is correct, I meant to say Finalize not Dispose in that last paragraph I'll update the answer to correct it.
• svick: @TrevorPilley, it's still not correct. Your current text makes it sound like Finalize() has something to do with IDisposable. Sure, when you have Finalize() you almost always implement IDisposable too, but it's not technically necessary.

Answer 4

Yes, always. Unless it's a class-level variable in which your type should then implement IDisposable and dispose of them in its Dispose() method.

So, option 2.

• Jesse C. Slicer: Well, except HttpClient. Because a rule wouldn't be a rule without an exception somewhere: aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong