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

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.