6

If you came across some C# code like this with nested using statements/resources:

using (var response = (HttpWebResponse)request.GetResponse())
{
    using (var responseStream = response.GetResponseStream())
    {
        using (var reader = new BinaryReader(responseStream))
        {
            // do something with reader
        }
    }
}

Is it safe to replace it with something like this?

using (var reader = new BinaryReader(((HttpWebResponse)request.GetResponse()).GetResponseStream()))
{
    // do something with reader
}

The example above is just an example of nested disposable resources, so forgive me if it's not exactly correct usage. I'm curious if when you dispose the outermost resource (the BinaryReader in this case), if it will recursively dispose its children for you, or if you need to explicitly dispose each "layer" with separate using statements? E.g. if you dispose the BinaryReader, is it supposed to dispose the response stream, which in turn disposes the response? Thinking about that last sentence makes me think you actually do need the separate using statements, because there's no way to guarantee that a wrapper object would dispose of the inner object. Is that right?

Andy White
  • 86,444
  • 48
  • 176
  • 211

5 Answers5

13

You should just stack your using statements - it has the desired effect you are looking for:

using (var response = (HttpWebResponse)request.GetResponse())
using (var responseStream = response.GetResponseStream())
using (var reader = new BinaryReader(responseStream))
{
    // do something with reader
}
Ken Richards
  • 2,937
  • 2
  • 20
  • 22
  • This is, essentially, the same code with different formatting (just removing the braces). – Reed Copsey Oct 19 '10 at 23:25
  • How does this improve things, that is effectively what the OP had just minus the curly brackets which were optional for the first two `using` statements. – slugster Oct 19 '10 at 23:27
  • Yes it's the same, but compared to what the author wanted to replace the original with, this is a lot easier to read and maintain – Ken Richards Oct 20 '10 at 00:08
12

You need the separate using statements.

In your second example, only the BinaryReader will get disposed, not the objects used to construct it.

In order to see why, look at what the using statement actually does. It takes your second code, and does something equivalent to:

{
    var reader = new BinaryReader(((HttpWebResponse)request.GetResponse()).GetResponseStream());
    try
    {
      // do something with reader
    }
    finally
    {
        if (reader != null)
            ((IDisposable)reader).Dispose();
    }
}

As you can see, there would never be a Dispose() call on the Response or ResponseStream.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • You need them, but I don't think they need to be nested. I think they can just be stacked on top of each other. – Matt H Oct 19 '10 at 23:24
  • @Matt: Yes, but they must all three exist. – Reed Copsey Oct 19 '10 at 23:24
  • `BinaryReader.Close` closes the underlying stream. And since `Stream.Close` ends up calling `Stream.Dispose`, there's no need to close the stream after closing the reader. See http://msdn.microsoft.com/en-us/library/system.net.httpwebresponse.aspx and http://msdn.microsoft.com/en-us/library/system.io.stream.close.aspx. – Jim Mischel Oct 19 '10 at 23:32
  • BinaryReader takes ownership of the stream and closes it when it is itself closed or disposed. So technically you don't need to clean up the stream--the reader does it for you--but having all three in using statements may prevent you and your colleagues from second-guessing it every time they see it... – Curt Nichols Oct 19 '10 at 23:33
  • @Jim: That still leaves the HttpWebResponse relying on the Garbage collector, though... – Reed Copsey Oct 19 '10 at 23:33
  • (Though it is true that the Stream doesn't need to be handled.. it doesn't hurt to handle it, and as a general rule, it's better to dispose than not...) – Reed Copsey Oct 19 '10 at 23:34
  • 1
    @Reed: But you don't need to dispose the HttpWebResponse if you close the stream. See http://msdn.microsoft.com/en-us/library/system.net.httpwebresponse.aspx. "You must call either the Stream.Close or the HttpWebResponse.Close method to close the response and release the connection for reuse. It is not necessary to call both Stream.Close and HttpWebResponse.Close, but doing so does not cause an error." – Jim Mischel Oct 19 '10 at 23:40
  • @Reed: I agree that as a general rule it's better to dispose than not. However, in this particular case it doesn't have to be done. – Jim Mischel Oct 19 '10 at 23:41
  • 1
    @Jim: Yes - I was taking this code as an example of the issue - in this case, it's not necessary to dispose of all of them - but as a general rule, it should be done. However, the same can be said of any properly written IDisposable - you never should have to dispose of them, as the finalizer **should** handle it, but it's still good practice. – Reed Copsey Oct 19 '10 at 23:42
  • @Jim: Thanks for the info on this specific case ;) – Reed Copsey Oct 19 '10 at 23:43
  • (For anybody reading these comments - also note that it should never be dangerous to Dispose() of every IDisposable, even if you call it multiple times or call it when it's technically unnecessary like above. If you are uncertain as to whether to dispose, it's always safe to call Dispose() when you're finished.) – Reed Copsey Oct 19 '10 at 23:50
  • @Reed, thanks for the input, your last comment makes sense to me. Things might dispose of inner resources in their Dispose or Close, but it's always safest to explictly Dispose/Close of every disposable/closable. Thanks – Andy White Oct 20 '10 at 01:09
3

FWIW, here's another way to spell your original example which may satisfy any dismay over nesting:

using (var response = (HttpWebResponse)request.GetResponse())
using (var responseStream = response.GetResponseStream())
using (var reader = new BinaryReader(responseStream))
{
    // do something with reader
}

Whether the reader disposes of the stream is really a function of the reader, not the 'using'. As I recall that is often what the behavior with readers is--they take ownership of the stream and dispose of it when the reader is itself closed. But the form I've provided above should be fine.

Curt Nichols
  • 2,757
  • 1
  • 17
  • 24
2

According to the documentation, BinaryReader.Close will close the underlying stream. http://msdn.microsoft.com/en-us/library/system.io.binaryreader.close.aspx

Also, according to the documentation for HttpWebResponse, you either have to close the underlying stream or dispose the response. http://msdn.microsoft.com/en-us/library/system.net.httpwebresponse.aspx

So the second example you provided would work.

Jim Mischel
  • 131,090
  • 20
  • 188
  • 351
  • Thanks for the answer... I suspected there were other possibilities. I guess when in doubt, it's best to just Close or Dispose everything, unless you know what each type does. – Andy White Oct 20 '10 at 01:10
0

I posted this elsewhere, but separating your declarations by comma seems to treat each statement separated this way as a new declaration and disposes of them.

using (IType1 a = new Type1(), b = new Type1()){}

This however means your objects must be of the same type. You could call them like

using (IDisposable a = new Type1(), b = new Type2()){}

But then of course, you only have access to IDisposable methods, without casting the object, which is kinda dumb. So instead, I believe you can use

using (var a = new Type1(), b = new Type2()){}

This appears to give you the correctly typed object references allowing you access to the proper method of the allocated type, and disposes of both objects created. If anyone knows why I'm not right, please let me know because this appears to work for me? (I know this question is really old n stuff, but it's all I could find while searching for this answer myself)

hcp
  • 486
  • 1
  • 3
  • 11