On this code:
public static string Base64FromFileName(string fileName)
{
try
{
FileInfo fInfo = new FileInfo(fileName);
long numBytes = fInfo.Length;
FileStream fStream = new FileStream(fileName, FileMode.Open, FileAccess.Read);
BinaryReader br = new BinaryReader(fStream);
byte[] bdata = br.ReadBytes((int)numBytes);
br.Close();
fStream.Close();
return Convert.ToBase64String(bdata);
}
catch(Exception e)
{
throw e;
}
}
...I get, courtesy of Visual Studio's Code Analysis tool, the warning, "Do not dispose objects multiple times...To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object" on the "fStream.Close();" line.
Why? Is fStream disposed in the line above, where the BinaryReader is closed?
Wouldn't I be better off refactoring it like this anyway:
. . .
using (FileStream fStream = new FileStream(fileName, FileMode.Open, FileAccess.Read))
{
using (BinaryReader br = new BinaryReader(fStream))
{
byte[] bdata = br.ReadBytes((int)numBytes);
} //br.Close();
} //fStream.Close();
. . .
?
BinaryReader.Close
also closes the underlying stream, so this would indeed cause the stream to be disposed of twice. But that's not a real problem, disposing twice doesn't hurt.
You could write this much better as
using (var fs = new FileStream(fileName, FileMode.Open, FileAccess.Read))
using (var br = new BinaryReader(fs, new UTF8Encoding(), true))
{
return Convert.ToBase64String(br.ReadBytes((int)numBytes));
}
This is the bomb-proof version:
- Anything that is successfully constructed is guaranteed to be disposed
- You won't dispose of the stream twice because the boolean
leaveOpen
argument on the BinaryReader
constructor ensures that disposing (closing) it won't also close the stream
Code Analysis is right; Code Analysis is wrong.
Yes, you're closing the FileStream twice. This is harmless. So is disposing it twice. Multiple disposal happens. It is the responsibility of the developer of a disposable component to handle multiple disposal properly and without throwing exceptions1.
However, while calling Dispose()
on a disposed FileStream
is a no-op by convention, the same isn't true of your code, which calls Close()
on a disposed stream. Don't do that.
Your suggested fix with the nested using is fine.
1 The contract for IDisposable.Dispose
requires:
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. Instance methods other than Dispose
can throw an ObjectDisposedException
when resources are already disposed.
The formal term for this behavior is idempotence.