Stream.Dispose or stream=null?

Question

I've have some code similar to this:

HttpWebRequest req;
HttpWebResponse response;
Stream receiveStream = null;
StreamReader readStream = null;
try
{
    req = (HttpWebRequest)WebRequest.Create("someUrl"));
    req.Credentials = CredentialCache.DefaultCredentials;

    req.Method = "GET";

    response = (HttpWebResponse)req.GetResponse();
    receiveStream = response.GetResponseStream();
    readStream = new StreamReader(receiveStream, Encoding.Default);

    return readStream.ReadToEnd();
}
catch
{
    return "Error";
}
finally 
{
    readStream = null;
    receiveStream = null;
    response = null;
    req = null;
}

Should this code have readStream.Dispose() and responseStream.Dispose() instead of setting both to null?

Answer 1

It should have using [which calls Dispose()].

Answer 2

No, you should call Dispose or Close

Answer 3

Yes - you should explicitly call Dispose() on classes that implement IDisposable after you have used them - this ensures all their resources get cleaned up in a timely fashion. Wrapping the variable in a using() does the same thing (which adds wrapping code that calls Dispose for you):

using (StreamReader reader = new StreamReader()) {
    // do stuff with reader
}

Answer 4

Yes.

When you set to null, it only nulls the reference. It doesn't run any cleanup code the creator of the Stream class wrote.

You may also want to consider the using(){ } statement which handles this for you on IDisposable types.

Example:

using (MyDisposableObject mdo = new MyDisposableObject)
{
   // Do some stuff with mdo
   // mdo will automatically get disposed by the using clause
}

Answer 5

Yes. Pretty much anything that implements a Dispose() method must have its Dispose() method called. You can implicitly call it in a with the 'using' statement:

using(StreamReader stream = GetStream())
{
   stream.DoStuff();
}

Answer 6

It's almost always a mistake to set local variables to null, unless you want to actually use that value later on. It doesn't force garbage collection any earlier - if you're not going to read from the variable later, the garbage collector can ignore the reference (when not in debug mode).

However, it's almost always correct to close streams - ideally in a using statement for simplicity.

It's also almost always wrong to have a bare "catch" block like that. Do you really want to handle anything going wrong, including things like OutOfMemoryException?

I would rewrite your code as:

HttpWebRequest req = (HttpWebRequest) WebRequest.Create("someUrl"));
req.Credentials = CredentialCache.DefaultCredentials;
req.Method = "GET";

using (WebResponse response = req.GetResponse())
{
    using (StreamReader reader = new StreamReader(response.GetResponseStream(),
                                                  Encoding.Default))
    {
        return reader.ReadToEnd();
    }
}

Now if something goes wrong, the exception will be propagated to the caller. You might want to catch a few specific exceptions, but it's generally not a good idea to represent errors using a value which could have been a valid "normal" response.

Finally, are you really sure you want Encoding.Default? That's the default encoding of the local machine - you normally want the encoding indicated by the response itself.

Answer 7

There are a few gotchas in the .net libraries. Stream is one, and the other is much of the Imaging API. These entities that use certain system resources don't garbage collect the attached system resources.

If anything uses the IDisposable API, the best thing to do is wrap it in a using block, as people have pointed out above.

Read up on "using", and keep it in mind whenever you're dealing with file handles or images.

Answer 8

Really, the question has been answered but I do want to elaborate on one thing.

Any time an object implements the IDisposable interface, you should dispose it with the Dispose method or (even better) use the using statement.

If you are ever faced with this question in the future, just find out which interfaces it implements. Once you see IDisposable you know to dispose it.

Answer 9

Safest method:

try {
    HttpWebRequest request = (HttpWebRequest) WebRequest.Create("someUrl");
    request.Credentials = CredentialCache.DefaultCredentials;
    request.Method = "GET";
    using (HttpWebResponse response = (HttpWebResponse) request.GetResponse()) {
        using (StreamReader reader = new StreamReader(response.GetResponseStream(), Encoding.Default)) {
            return reader.ReadToEnd();
        }
    }
} catch {
    return "Error";
}

There's no need to dispose of the response.GetResponseStream() stream explicitly because the attached StreamReader will dispose it for you.

EDIT: I agree with the other answers - catching exceptions like that is very bad practice. I just left it in for comparison purposes. :-)

Answer 10

If you need to clear the stream use null; Otherwise, use the Dispose(); method if your application no longer requires the use of the stream.