c# Const vs. Readonly, Scope, Usings
-
I agree that TextReader is a bad example for other reasons: probably the constructor doesn't throw. And I saw your other response, and it doesn't change things: it clarifies that Dispose does indeed call close on the outerStream, but it doesn't say a thing about what happens if the constructor throws. My point is that with:
var outerStream = new AStream();
using (var innerStream = new AnotherStream(outerStream)) {...}If "new AnotherStream(outerStream)" throws you get to hold the undisposed baby as innerStream.Dispose() never gets called and the outerStream isn't closed. Can't comment on VS code analysis, as I only use Resharper, but just for fun, trying VS2015 code analysis (all rules on) on this code:
{
var outerStream = new MyStream(); // line 59
using (var innerStream = new MyStream(outerStream)) {
bool i = innerStream.CanRead;
Console.WriteLine("Read: " + i);
}
}
{
using (var outerStream = new MyStream())
{
using (var innerStream = new MyStream(outerStream))
{
bool i = innerStream.CanRead;
Console.WriteLine("Read: " + i);
}
} // Line 73
}[Line 59] Warning CA2000 In method 'Program.Main(string[])', call System.IDisposable.Dispose on object 'outerStream' before all references to it are out of scope. [Line 73] Warning CA2202 Object 'outerStream' can be disposed more than once in method 'Program.Main(string[])'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object. Bit schizophrenic: in the first case complains that Dispose isn't called and on the second complains that it's called twice :) Edit: - Moved Line 73 one line up... - VS code analysis agrees with me: the first warning says that with only one using it's not guaranteed that the outer stream gets closed. -(Re)thinking on this, VS code analysis is not smart enough: it knows that innerStream.Dispose() calls outerStream.Dispose() but doesn't know that Stream.Dispose() is (should always be) idempotent and can get called any number of times.
That's really really odd - is there any example of a Dispose() method throwing an ObjectDisposedException if called twice? Because if so, that's going against the guidelines for implementing a Dispose method. Dispose methods should never throw for exactly this reason (among others), so the Line 73 warning seems rather odd.
Blog: [Code Index] By Mike Marynowski | Business: Singulink
-
I can see your point but accounting for errors in Stream constructors, which you can catch, isn't close to the point I was trying to make. I was just implying that it's generally a good idea to use
usingbut is not always a straight forward case such as when using streams that one would assume derive from System.IO.Stream based on code symantics. What sets StreamReader and StreamWriter apart is that, while they behave exactly like a stream, they do not derive from System.IO.Stream. They derive from System.TextReader which in turn derives directly from System.MarshalByRefObject. So here we have two classes mimicking the behavior of an entire branch of classes but have subtle differences in implementation. Since most people would assume that StreamReader derived from IO.Stream, it can cause confusion when CA2202 messages warn that you're disposing of a object twice even thought you are actually following recommended best practices.if (Object.DividedByZero == true) { Universe.Implode(); } Meus ratio ex fortis machina. Simplicitatis de formae ac munus. -Foothill, 2016
Hmm... using the example on your other post (changed just enough to make the code compile):
byte[] data = new byte[1000];
MemoryStream memStream = new MemoryStream(data);
CryptoStream decStream = new CryptoStream(memStream, SHA1.Create(), CryptoStreamMode.Read);using (StreamReader reader = new StreamReader(decStream)) {
byte[] decryptedValue = Encoding.UTF8.GetBytes(reader.ReadToEnd());
Console.WriteLine(decryptedValue);
}We get a (for me accurate) CA2000 warning: object 'memStream' is not disposed along all exception paths. And... what's this, no warning about an undiposed decStream??? Let me check:
SHA1 sha1 = SHA1.Create();
byte[] data = new byte[1000];
using (MemoryStream memStream = new MemoryStream(data)) {
CryptoStream decStream = new CryptoStream(memStream, sha1, CryptoStreamMode.Read);StreamReader reader = new StreamReader(decStream); byte\[\] decryptedValue = Encoding.UTF8.GetBytes(reader.ReadToEnd()); Console.WriteLine(decryptedValue);}
No undisposed warnings with VS about this. Was going to say I could agree with this code, but no, even though it passes VS code analysis I disagree even more with it. Who can say what goes on in all the undisposed inner streams in the case of an exception? For me: a) (repeat from my other post) It seems that while VS2015 (and 2017, just tested) code analysis is smart enough to know that innerStream.Dispose calls outerSteam.Dispose it's not smart enough to know that Dispose in streams should be idempotent and should be able to be called several times. b) It's really strange there being a best practice that doesn't ensure every stream is closed in all code paths, including exceptions.
-
Hmm... using the example on your other post (changed just enough to make the code compile):
byte[] data = new byte[1000];
MemoryStream memStream = new MemoryStream(data);
CryptoStream decStream = new CryptoStream(memStream, SHA1.Create(), CryptoStreamMode.Read);using (StreamReader reader = new StreamReader(decStream)) {
byte[] decryptedValue = Encoding.UTF8.GetBytes(reader.ReadToEnd());
Console.WriteLine(decryptedValue);
}We get a (for me accurate) CA2000 warning: object 'memStream' is not disposed along all exception paths. And... what's this, no warning about an undiposed decStream??? Let me check:
SHA1 sha1 = SHA1.Create();
byte[] data = new byte[1000];
using (MemoryStream memStream = new MemoryStream(data)) {
CryptoStream decStream = new CryptoStream(memStream, sha1, CryptoStreamMode.Read);StreamReader reader = new StreamReader(decStream); byte\[\] decryptedValue = Encoding.UTF8.GetBytes(reader.ReadToEnd()); Console.WriteLine(decryptedValue);}
No undisposed warnings with VS about this. Was going to say I could agree with this code, but no, even though it passes VS code analysis I disagree even more with it. Who can say what goes on in all the undisposed inner streams in the case of an exception? For me: a) (repeat from my other post) It seems that while VS2015 (and 2017, just tested) code analysis is smart enough to know that innerStream.Dispose calls outerSteam.Dispose it's not smart enough to know that Dispose in streams should be idempotent and should be able to be called several times. b) It's really strange there being a best practice that doesn't ensure every stream is closed in all code paths, including exceptions.
Yes. I can say that the code does not handle disposing of the streams along all exception paths but, for some context, the only time a stream constructor will fail is when you pass it bad arguments or bad file paths. In well tested code, the only time a stream constructor should fail is for
StackOverflowExceptionorOutOfMemoryExceptionand your program is pretty much hosed at that point anyways. ;)if (Object.DividedByZero == true) { Universe.Implode(); } Meus ratio ex fortis machina. Simplicitatis de formae ac munus. -Foothill, 2016
-
With C# getting to version 7+ I wish I could have some basic improvments. Is it me or do you get confused by this? Why can't I say
const DateTime today = DateTime.Now;
I can see readonly for parameters and such, but I would be happy using const there too
void Doit(const MyObj arg) ...
For properties, why can't I hide the worker variable for the rest of the class?
public int PageNbr
{
int _worker = 9;get { return _worker;}
set { _worker = value; }
}For destructors, why can't you give me option to destroy right away? I hate disposing with all its using code bloat. How about a free keyword on a variable or something to get me out of the business of resource management. If you open a file and a DB you have to nest usings before you even get started doing some work! Or maybe I'm missing something?
- Pete