CODEWITHSUNDEEP
c#.net
Temple
Temple

Reputation: 1079

Stream.Dispose or stream=null?

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?

Upvotes: 11

Views: 4178

Answers (11)

Christian Hayter
Christian Hayter

Reputation: 31071

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

Upvotes: 2

jay_t55
jay_t55

Reputation: 11652

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.

Upvotes: 0

Henk Holterman
Henk Holterman

Reputation: 273274

Yes, Dispose() them.

Even better to do something like 

using (HttpWebResponse response = (HttpWebResponse)req.GetResponse() )
using (Stream receiveStream = response.GetResponseStream() )
using (readStream = new StreamReader(receiveStream, Encoding.Default) )
{
   return readStream.ReadToEnd();
}

A using(x) {} block will be rewritten (by the compiler)
as a try {} finally {x.Dispose();}

Note that the WebRequest is not IDisposable.

Also note that the following lines accomplish the same thing as all of your code:

using (var client = new System.Net.WebClient())
{
    client.Encoding = ...;
    client.Credentials = ...;
    return client.DownloadString("SomeUrl");
}

Upvotes: 9

Jimmy
Jimmy

Reputation: 91472

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

Upvotes: 27

Frank V
Frank V

Reputation: 25429

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.

Upvotes: 0

user122299
user122299

Reputation:

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.

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1500893

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.

Upvotes: 28

Aric TenEyck
Aric TenEyck

Reputation: 8032

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();
}

Upvotes: 8

Joel
Joel

Reputation: 19368

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
}

Upvotes: 4

thecoop
thecoop

Reputation: 46118

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
}

Upvotes: 1

Ray
Ray

Reputation: 1585

No, you should call Dispose or Close

Upvotes: 2

Related Questions