Do i need to Dispose Stream when i Pass it to IDisposable class?

Question

I wrote a piece of code. I want to make sure that I am Disposing an Object in right way.

I have a Disposable Class like this Which is used to read some data from unmanaged resource.

class MyFileReader : IDisposable
{
    private readonly FileStream _stream;

    public MyFileReader(FileStream stream)
    {
        _stream = stream;
    }

    public void Dispose()
    {
        _stream.Dispose();
    }
}

Currently in my program I Dispose objects like this.

        using (FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read))
        {
            using (MyFileReader reader = new MyFileReader(stream))
            {
                //...
            }
        }

This seems ok to me. later I have noticed That Classes are passed by reference so maybe if I dispose one of them there is no need to dispose the other one.

My question is Can i Do Something like this?

        using (FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read))
        {
            MyFileReader reader = new MyFileReader(stream);
            // Remove IDisposable from MyFileReader and stream will close after using.
        }

Or this one?

        FileStream stream = new FileStream(filename, FileMode.Open, FileAccess.Read, FileShare.Read);
        // stream will close after using.
        using (MyFileReader reader = new MyFileReader(stream))
        {
            //...
        }

Tip: [CA1063: Implement IDisposable correctly](https://msdn.microsoft.com/en-us/library/ms244737.aspx) — Sebastian Schumann, Jul 07 '15 at 05:15
If you pass a stream in via the constructor then **do not** dispose of it as it may be used outside of your class afterwards. If your class creates the stream then it is responsible for disposing it. — Enigmativity, Jul 07 '15 at 05:23
@Enigmativity Have a look at Alexei Levenkovs answer. The framework is currently disposing a stream that it gets. Maybe have a look at the `BinaryReader` that disposes the stream got in constructor. — Sebastian Schumann, Jul 07 '15 at 05:26

score 3 · Accepted Answer · answered Jul 07 '15 at 05:14

3

Yes, you can write code like that.

But, no, you should not do that.

Your class looks like on of XxxxReader classes that by convention own the stream they read from. As result your MyFileReader class is expected to dispose the inner stream. You also normally expected to dispose each and every disposable object when you know that such object's lifetime is over.

Note that sometimes it lead to multiple Dispose calls on some objects (which should be expected by implementations of IDisposable). While it may sometimes lead to Code Analysis warning it is better than missing Dispose calls if one routinely tries to "optimize" number of calls to Dispose by skipping some.

Alternative approach is to expose method that reads content which by convention is not expected to take ownership of stream/reader like:

 using(stream....)
 {
   var result = MyFileReader.ReadFrom(stream);
 }

answered Jul 07 '15 at 05:14

Alexei Levenkov

98,904
14
127
179

Do you have any official sources for the fact of _XxxxReader classes that by convention own the stream they read from_ and _method that reads content which by convention is not expected to take ownership of stream/reader_? The framework code is acting like that but I haven't found any official statement about that and I'm looking for it. – Sebastian Schumann Jul 07 '15 at 05:18
an `ObjectDisposedException` might be a familiar example here. – Amit Kumar Ghosh Jul 07 '15 at 05:22
1

@Verarind I don't think I've seen any "official source" for that. That's why I say "by convention" - most code I've seen (indeed starting with framework itself) behaves that way (which is also somewhat required due to [viral nature of IDisposable](http://stackoverflow.com/questions/661815/how-do-you-prevent-idisposable-from-spreading-to-all-your-classes?rq=1)] ) and hence it makes it easier to reason about code that simply follows the same pattern. – Alexei Levenkov Jul 07 '15 at 05:24
2

@AmitKumarGhosh You mean "there are implementations of IDisposable that does not follow the rules"? http://stackoverflow.com/questions/5306860/should-idisposable-dispose-be-made-safe-to-call-multiple-times , MSDN: "If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times." – Alexei Levenkov Jul 07 '15 at 05:27
So calling Dispose more than once doesn't make problem.so for insurance my first code seems ok. but if stream is used only once and we have one instance of `MyFileReader` then there is no need to dispose stream twice. am i right? – M.kazem Akhgary Jul 07 '15 at 05:31
I was under the impression calling `Dispose` is similar to calling any other methods on the object. I'll remember this is wrong. Thanks @Alexei. – Amit Kumar Ghosh Jul 07 '15 at 06:01
1

@M.kazemAkhgary yes, all 3 versions of the code dispose stream, but first on is safe. First one dispose it twice but clearly *correct when you look at the code*, two others dispose stream too but every person looking at the code will have to spend non-trivial amount of time trying to figure out if code is indeed properly disposes all actually disposable object. Additionally if there is ever change to your `MyFileReader` class introducing more disposable parts into class then second version (one that just disposes stream) will start leaving non-dispose object till GC comes around. – Alexei Levenkov Jul 07 '15 at 06:11

score 1 · Answer 2 · answered Jul 07 '15 at 05:13

If MyFileReader is accessing some unmanaged resource and you need the Disponse method to be called explicitly after this code block, then you have to stick with your current implementation.

In the second implementation, the Dispose method will not be called for the MyFileReader object. (until you probably call it in the destructor which you don't know when that would be called)

If you do not like the nested using, then you can probably go with the second alternative and in the Dispose() method implementation of MyFileReader class, explicitly dispose the Stream. If this stream is only use by MyFileReader, then this is a good practice to have MyFileReader manage its lifecycle and dispose it.

score 1 · Answer 3 · answered Jul 07 '15 at 06:33

Many framework stream-wrapping classes have constructor overloads with leaveOpen parameter that controls stream disposing behavior.
Examples include StreamReader, BinaryReader, GZipStream.

There is also property approach, SharpZipLib example:

using (var zip = new ZipInputStream(stream) { IsStreamOwner = false }) { ... }

Do i need to Dispose Stream when i Pass it to IDisposable class?

3 Answers3