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 ?
Dispose
is meant to be idempotent by design, so calling it repeatedly should have no effect - Douglas 2012-04-05 21:52
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
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
ReadFile()
accepts for example a string fileName
and Opens and Closes the Stream, orYour 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.
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
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
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
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.
Who opens the door must remember to close it.
So it's better to close the stream in the method who has opened it.
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
}
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
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.
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.
ReadFile
to document whether or not it closes the stream. For example, the constructor forStreamReader
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