Closing a stream in function, a bad idea?

Go To StackoverFlow.com

7

I wrote some code today and It was changed by another developer who said it was more safe. I am not sure this is right as I cannot see the advantage of what was done here are some code examples

public byte[] ReadFile(Stream stream)
{
    byte[] result = null;

    try
    {
       // do something with stream
       result = <result of operation>
    }
    finally
    {
        stream.Close();
    }

    return result;
}

this was changed to

public byte[] ReadFile(Stream stream)
{
    byte[] result = null;

    // do something with stream
    result = <result of operation>

    return result;
}

I am quite new to c# should the stream not be closed when you are finished with it ?

2012-04-05 20:17
by NoName
Not that in either case it is beneficial for your ReadFile to document whether or not it closes the stream. For example, the constructor for StreamReader that takes a stream specifically says that it disposes of that stream, so the caller shouldn't dispose of it again - Servy 2012-04-05 20:29
@Servy: It is more accurate to say that the caller needn’t dispose of it again. Dispose is meant to be idempotent by design, so calling it repeatedly should have no effect - Douglas 2012-04-05 21:52
@Douglas It's still important to know if a method disposes of the passed in IDisposable because the caller may wish to continue to use it. If you pass it to someone who disposes of it and you use it again, BAM - Servy 2012-04-05 22:31
@Servy: Yes, definitely, I didn’t contest that. I just pointed out that it’s allowed for the caller to dispose of the stream again - Douglas 2012-04-06 14:35


11

In general the first version would be a bad design.

Yes, the Stream should be closed but preferably by the same code (same method) that opened it. That is called separation of concerns and it makes mistakes and confusion much less likely.

So either

  • your ReadFile() accepts for example a string fileName and Opens and Closes the Stream, or
  • you leave it to the caller.

Your method (the 2nd version) should be used in this manner:

using (var reader = new FileReader(...))
{
  // maybe some pre-reading
  var r = ReadFile(reader);
  // maybe some post-reading
}

Note that the 2nd approach also makes the method more reusable.

2012-04-05 20:19
by Henk Holterman
this makes sense now thanks very much I went back and looked and realised that I already had the call wrapped in a using so thought that was the only reason not to do this, it seems you have opened my eyes. Thank you - NoName 2012-04-05 20:28
@Henk Holterman: I agree completely with your statement on separation of concerns, but unfortunately this design appears to be broken by .NET, at least in the case of constructors. Passing a Stream to the constructor of a reader (e.g. StreamReader), writer (e.g. BinaryWriter), or even another stream (e.g. GZipStream) would cause the original stream to be disposed when the other reader/writer/stream is - Douglas 2012-04-05 21:33
@Douglas - yes and that is a much debated design. Sometimes convenient, sometimes a nasty surprise. But at least it is moving ownership between objects, not between methods - Henk Holterman 2012-04-05 21:46
@HenkHolterman: Fair point re transfer of ownership; however, it does imply that one shouldn’t pass a Stream to a StreamReader unless one “owns” the stream, making the use of StreamReader ineligible for situations such as the OP’s method. I just wish we didn’t have to resort to third-party workarounds such as NonClosingStreamWrapper so often - Douglas 2012-04-05 22:02
@HenkHolterman: The proper solution for StreamReader would have been to have a constructor parameter to specify whether it was taking ownership, since there are times when each approach can be massively advantageous - supercat 2012-04-05 23:32


6

There is no right answer on this question, cause it depends on the your app architecture.

I would say, yes, cause if in this function stream is not created, but only used, so closing of it let's let up to the caller.

But I repeat, it depends on your app architecture.

2012-04-05 20:20
by Tigran


5

Who opens the door must remember to close it.

So it's better to close the stream in the method who has opened it.

2012-04-05 20:20
by Steve


3

Typically the creator of the Stream should be the one to close it, ideally by disposing it with a using block:

using (var myStream = getMeAStream()) {
    ReadFile(myStream);

    // If you want to be really sure it is closed:
    myStream.Close();
    // Probably not neccessary though, since all 
    // implementations of Stream should Close upon Disposal
}
2012-04-05 20:21
by Chris Shain


1

The stream has been passed into this function by something else, you are not responsible in the function to close it , it is up to the calling code to deal with that issue.

The reason is that another operation (like reset or another read) may need to be done externally to this function and if you close it you will cause an exception.

I used to refactor code like this several times a day until finally someone listened to me.

I have seen at least one serious bug caused by this sort of code, so its a bad idea in general

2012-04-05 20:22
by krystan honour


0

The code review was right - never do such a thing (or if something like that is required by an outside code then that code is probably not designed properly - there are exceptions but for things like streams disposing there almost never is, and some default 'patterns' are always to be followed).
in most cases use streams like this (from 'outside')...

using(Stream stream = new ...)
{
    ...call your method
}

(or e.g. reader is disposing of it - but it's recommended that you do something like this in either case - equivalent is using a 'finally' block, but boils down to the same thing)

...basically the calling function would never know if you disposed of it 'inside' or not - until it crashes out. If 'both parts' agree on something like that, well it's still not acceptable but could go unpunished.

2012-04-05 20:21
by NSGaga


0

It's not simply a C# problem. After the call of function "ReadFile" you need to use the stream for other operations? If no you could choose to close the stream after you read the file. It's better closing streams when you finish to use them, but generally I prefer to close them in the same context than opens them, because it's there the place where you know if you need stream for other operation:

Stream s = open_the_stream()
try {
  ReadFile(s)...
} finally {
  s.Close();
}

In any way, close stream when you terminate to use them.

2012-04-05 20:25
by dash1e
thanks this clarifies the situation mor - NoName 2012-04-12 21:20
Ads