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'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 2

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