Code Anlysis Rule CA2000 / CA2202

Question

I am trying to ensure my coding follows correct disposal of objects so I am enforcing these rules as errors. But I am having trouble with this section of code

using System;
using System.IO;
using System.Runtime.Serialization;
using System.Xml;

class MyClass
{  
    public String ToXml()
    {
        var objSerializer = 
            new DataContractSerializer(GetType());
        var objStream = new MemoryStream();
        StreamReader objReader;

        String strResult;
        try
        {
            // Serialize the object
            objSerializer.WriteObject(objStream, this);

            // Move to start of stream to read out contents
            objStream.Seek(0, SeekOrigin.Begin);

            objReader = new StreamReader(objStream);

            try
            {
                // Read Contents into a string
                strResult = objReader.ReadToEnd();
            }
            finally
            {
                objReader.Dispose();
            }
        }
        finally
        {
            if (objStream != null)
            {
                // objStream.Dispose();
            }
        }

        return strResult;
    }
}

If I comment out objStream.Dispose() I get CA2000 as I am not disposing the object but if I remove the comment it then says I am disposing more than once.

What else is disposing the object? or am I just doing this wrong when dealing with multiple streams?

Answer 1

This scenario has been annoying me as well now. Every couple of years I decide to refresh myself of the code analysis "rules" by running fxcop or the now built-in code analysis in Visual Studio.

I originally wrote this code, thinking I was being a good citizen for properly using usings to dispose:

using (MemoryStream msDecrypt = new MemoryStream())
{
    using (CryptoStream csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Write))
    {
        csDecrypt.Write(eop.m_Ciphertext, 0, eop.m_Ciphertext.Length);
    }

    decrypted = msDecrypt.ToArray();
}

This block of code results in a CA2202 "Do not dispose objects multiple times" The great irony about this rule, is that it's not really about a problem with your code, as much as it is protecting you about a problem in others code. Microsoft has always had a decent amount of documentation about how the Dispose pattern (http://msdn.microsoft.com/en-us/library/b1yfkh5e(v=vs.110).aspx) should be implmented. However, by looking at the details of this code analysis rule (http://msdn.microsoft.com/en-us/library/ms182334.aspx) it reveals the purpose of this rule

"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."

In short, this rule is all about protecting yourself from people who don't follow the rules.

Naturally I modified the code to look like this:

MemoryStream msDecrypt = new MemoryStream()    
//using (MemoryStream msDecrypt = new MemoryStream())
//{
    using (CryptoStream csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Write))
    {
        csDecrypt.Write(eop.m_Ciphertext, 0, eop.m_Ciphertext.Length);
    }

    decrypted = msDecrypt.ToArray();
//}

Now everybody on this stack overflow post is painfully aware of the new problem, our friend CA2000 "Dispose objects before losing scope" ... so at this point I just face palmed for a minute. Did a few Google searches, and found this post. That's when it dawned on me, to pass for both CA rules, you need to ensure everything is disposed once and only once for all code branches. So I set out to do this, which isn't a hard problem once you realize this is what you need to do.

Naturally, the code evolved to this:

MemoryStream msDecrypt = null;
CryptoStream csDecrypt = null;

try
{    
    msDecrypt = new MemoryStream();
    csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Write);

    csDecrypt.Write(eop.m_Ciphertext, 0, eop.m_Ciphertext.Length);
    csDecrypt.FlushFinalBlock();
    decrypted = msDecrypt.ToArray();
}
finally
{
    if (csDecrypt != null)
    {
        csDecrypt.Dispose();
    }
    else if (msDecrypt != null)
    {
        msDecrypt.Dispose();
    }

}

Finally, I had code that didn't result in a CA2000 or CA2202. The moral of the story is that the USING statement, is a lot less valuable than it was in the past now that code analysis rules have evolved in this way.

There are a few different ways you can write the code to make this work, I just chose a way that doesn't mix explicit calls to dispose with using statements, because I believe this to be simpler to read as well as structured in a way that would prevent someone from just going and wrapping another using around it, resulting in the originally issue unknowingly.

Answer 2

If you dispose the StreamReader, you are also disposing the underlying stream.

If you comment out objStream.Dispose() then you run into the chance of something throwing an exception before you even get to the nested try block - which will result in your stream not getting disposed.

There's a nice explanation here: Does disposing streamreader close the stream?

Answer 3

This is not the answer, but you might find this code more readable:

public String ToXml()
{
    var objSerializer =
        new DataContractSerializer(GetType());

    using (var objStream = new MemoryStream())
    {
        //  Serialize the object
        objSerializer.WriteObject(objStream, this);

        // Move to start of stream to read 
        // out contents
        objStream.Seek(0, SeekOrigin.Begin);

        using (var objReader =
            new StreamReader(objStream))
        {
            // Read Contents into a string
            retirm objReader.ReadToEnd();
        }
    }
}