Dela via

The worst Code Analysis Rule that’s ‘recommended’ - CA2202

  • Artikel

CA2202: Do not dispose objects multiple times

How I hate this rule!

Firstly, the rule is predicated on the belief that other people will write broken code and you will have to use it. Broken how? The documentation for IDisposable.Dispose() says that it must be safe to call IDisposable.Dispose() multiple times on the same object:

“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.”

It also says that only methods that are not Dispose() can throw ObjectDisposedException.

So why then CA2202? Well, let us read its documentation:

“A method implementation contains code paths that could cause multiple calls to IDisposable.Dispose or a Dispose equivalent, such as a Close() method on some types, on the same object. “

”A correctly implemented Dispose method can be called multiple times without throwing an exception. However, this is not guaranteed and to avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object.”

What do they mean it’s not guaranteed? It is guaranteed already! IDisposable.Dispose() says so. And calling Dispose() on a object that has had Close() called should be perfectly safe too! (And would you ever do it in the other order?)

So yes, the rule as it exists is pretty flawed in concept. Because it’s telling you to fix the wrong thing. What they should do instead is help enforce the other side of the Dispose() contract. CAXYZZ: do not ever do anything that makes your Dispose() method unsafe to call multiple times. Specifically don’t write code that can throw ‘obvious’ exceptions like NullReferenceException(), ArgumentNullException(), or ObjectDisposedException() from your Dispose() method.

However, it’s not just that this rule is telling you to fix the wrong thing… if you try to follow it’s recommendations, you will often actually a) make your code look wrong (aside: Joel Spolsky is he who by his writing taught me to think about whether code ‘looks wrong’. I think this is one hell of a great concept to understand, and it really doesn’t require hungarian notation.) b) break your code.

What, break my code? Yes. Look at this diff:

// Encrypt the data as an array of encrypted bytes in memory.
-using (var memoryStream = new MemoryStream())
-{
-    using (var encStream = new CryptoStream(memoryStream, cryptoTransform, CryptoStreamMode.Write))
-    {
-        using (var sw = new StreamWriter(encStream, new UTF8Encoding()))
-        {
-             sw.Write(plainText);
-        }
-      }
-}
+var memoryStream = new MemoryStream();
+var encStream = new CryptoStream(memoryStream, cryptoTransform, CryptoStreamMode.Write);
+using (var sw = new StreamWriter(encStream, new UTF8Encoding()))
+{
+    // Write the plaintext to the stream.
+    sw.Write(plainText);
+    sw.Flush();
+    encryptedBuffer = memoryStream.ToArray();
+}

This is an attempt to satisfy CA2202. It is also a WRONG attempt to satisfy CA2202, since it introduces a bug. The bug is… (spoiling the puzzle - if you wanted to figure it out yourself…) the encrypted data returned is now empty when we encode a short test string. We actually also need to call encStream.FlushFinalBlock() in order to get the correct result.

Now luckily this bug was caught straight away by unit tests. But really why did introducing this bug happen? There is nothing wrong with the original code with 3 nested usings! The only reason the bug was introduced, is because we attempted to satisfy code analysis rule CA2202.

 

PS - when originally posted I forgot to explain what I meant about the code looking wrong.

I simply meant that it looks wrong having something IDisposable not be in a using block. When it's in a using block you automatically know that it will be disposed without doing any further reading of the code. If it's *not* in a using block, then you have to look at the code path below and consider whether there are any troublesome edge cases, whether a try/finally should have been used, and so on.

Comments

  • Anonymous
    February 12, 2014
    The comment has been removed

  • Anonymous
    February 13, 2014
    I would argue that the problem with the initial code is that IDisposable is being used to do something other than freeing unmanaged resources.  MemoryStream doesn't use unmanaged resources and from what I can see neither does CryptoStream. These classes are forced to implement IDisposable because Stream requires it. This use of Using is borrowing the C# statement for doing regular housekeeping stuff instead of what it was originally intended for. The fact that CryptoStream uses Dispose to call FlushFinalBlock is the root problem IMHO.  

  • Anonymous
    February 13, 2014
    @Darrel Personally I'm quite happy that I can just do using() statements with Cryptostream, StreamWriter etc instead of needing to look up how to flush the final block and underlying writer. But I think that's not relevant to the overall argument of the post. It is relevant however that even if there is an underlying managed resource, it should still be fine to call Dispose() on it as many times as you want. A Dispose() implementation should always shield you from the pain of counting how many times you called it.

  • Anonymous
    February 13, 2014
    @tilovell09  Most of the time it is harmless.  With HttpClient dispose causes the underlying TCP/IP connection in the ServicePointManager pool to be forcibly closed causing a potentially significant perf hit. However, you are correct.  Double dispose should not cause a problem.

  • Anonymous
    April 23, 2014
    Nice post. BTW you can also do this to considerably reduce nesting: -using (var memoryStream = new MemoryStream()) -using (var encStream = new CryptoStream(memoryStream, cryptoTransform, CryptoStreamMode.Write)) -using (var sw = new StreamWriter(encStream, new UTF8Encoding())) -{

  •    sw.Write(plainText); -} That generally reads better, especially if you have to have other nesting constructs within the using.

  • Anonymous
    September 16, 2015
    Mmmmh... Way too radical... Yes your corrected code to respect the CA is wrong. But if you use a try finally dispose properly, it s correct. Yes it may be ugly. You can't just write code without considering what can break it. And you can't just write code without considering possible changes in the future. And you can't always fix the other side of the dispose contract. "When to Suppress Warnings Do not suppress a warning from this rule. Even if Dispose for the object is known to be safely callable multiple times, the implementation might change in the future."

  • Anonymous
    August 24, 2016
    As well as the acknowledged issues, one more I have is Close may not actually be the equivalent of Dispose, and, like the MSDN warning not to suppress this rule says, future changes to Close may make this so even if it's the same now.