views:

207

answers:

6

Do I have to do this to ensure the MemoryStream is disposed of properly?

  using (MemoryStream stream = new MemoryStream(bytes))
  using (XmlReader reader = XmlReader.Create(stream))
  {
    return new XmlDocument().Load(reader);
  }

or is it OK to inline the MemoryStream so that it simply goes out of scope? Like this?

  using (XmlReader reader = XmlReader.Create(new MemoryStream(bytes)))
  {
    return new XmlDocument().Load(reader);
  }
+3  A: 

This really depends on the Dispose() of XmlReader. It would take some work to figure out exactly what it does. I personally write code like the first sample. If you new something, then it is your responsibility to dispose it. You shouldn't expect others to take care of it for you (although they may).

Bryan
Thanks Bryan,So do I. I just wondered if an instance declared inline might always be regarded as part of the containing IDisposible and thus always be disposed of when the container's dispose method is called.I would want a general rule that applied to all IDisposibles though, I don't want to write it one way for some and a different way for others.
Colin
You seem to think that "inline" as in "in a using block" is something known to the code. It is not. using () {} is just syntactical sugar for try {} finally {}, making sure that .Dispose() is called.
Benjamin Podszun
+11  A: 

The XmlReader does not by default (but see Colin's and dh's suggestion) assume that it is the only one using a stream, so the first option is the only Dispose safe one.

Pontus Gagge
Unless you use XmlReaderSettings.CloseInput Property as dh suggests
Colin
+11  A: 

As a general rule, yes, you should write the code as in the first example.

There are some classes that take ownership of the object passed to it, so that when you dispose the outer object, it automatically disposes of the inner object for you, but that's the exception to the rule.

In any case, calling Dispose more than once is supposed to be safe. That is, objects should implement that so that it is safe, only doing the work the first time.

So as a general rule, go with the first syntax.

Now, for the specified example, it shouldn't really matter, as a MemoryStream isn't really holding on to any resources that needs to be disposed of, but there is a problem with that expectation too. If you know that a given version of an object doesn't use a resource, so it's safe to ignore the Dispose, then if that object in the future gains such a resource, you suddenly gain a leak.

Unless you're seeing some adverse effect with the given code, like adding too much overhead, then I would simply not worry about it.

Lasse V. Karlsen
I like the "simply not worry about it"--we're supposed to be able to (mostly) forget about memory management, right? Sometimes, it feels like we've just repackaged the problem as *IDisposable*...
Dan
`IDisposable` isn't about memory management. It's about _resource_ management. GC doesn't free you from worrying about resource management.
John Saunders
I agree with both comments to an extent: yes, it is about *resource* management, and `IDisposable` when used properly does give us a feeling that we *simply not worry about it*. `IDisposable`, when used properly and efficiently is simply a to assist the GC without severely affecting it's supposed efficiency. IMO, how can anything that is described as behaving *indeterminately* truly be efficient?
dboarman
+2  A: 

You're talking about two different things:

  1. From a design a best-practices perspective, should you always dispose an object for which you're responsible? Yes
  2. Are you going to experience a memory leak following the pattern you show in the second example? No, if for no other reason than the fact that MemoryStream.Dispose doesn't actually do anything.
Adam Robinson
+4  A: 

There is an option to use XmlReaderSettings and set CloseInput to true like this

var reader = XmlReader.Create(new MemoryStream(), new XmlReaderSettings {CloseInput = true});

Here: XmlReaderSettings.CloseInput Property

dh
A: 

Memory streams don't actually require calling Dispose(). However the question is still valid in general as other kinds of streams require Dispose().

Joshua