25

Recently I switched on additional code analyses rules. To my surprise I saw a violation in a place I was always considering as the best practice. If I have two nested disposables I am putting two using statements like this:

    using (StringReader strReader = new StringReader(xmlString))
    using (XmlReader xmlReader = XmlReader.Create(strReader))
    {
        result.ReadXml(xmlReader);
    }

This also corresponds to the high rated Q&A Nested using statements in C#

The violation I get states following:

Warning 18  CA2202 : Microsoft.Usage : Object 'strReader' can be disposed more
than once in method '????'. To avoid generating a System.ObjectDisposedException
you should not call Dispose more than one time on an object.: Lines: ??

What I did was an intuitive try and error, thinking that close of outer stream will also probably dispose the inner one I quick fixed my code like this:

    using (XmlReader xmlReader = XmlReader.Create(new StringReader(xmlString)))
    {
        result.ReadXml(xmlReader);
    }

Hura! The warning is gone. But, tada! The new one occurred:

Warning 18  CA2000 : Microsoft.Reliability : In method '????????', object 
'new StringReader(xmlString)' is not disposed along all exception paths. Call
System.IDisposable.Dispose on object 'new StringReader(xmlString)' before all 
references to it are out of scope.

Then I found a very ugly solution: 

    {
        StringReader strReader = null;
        try
        {
            strReader = new StringReader(xmlString);
            using (XmlReader xmlReader = XmlReader.Create(strReader))
            {
                strReader = null;
                result.ReadXml(xmlReader);
            }
        }
        finally
        {
            if (strReader != null) strReader.Dispose();
        }
    }

As a very last step (like every good programmer) I looked into help page for CA2202 and to my surprise exactly my last UGLY solution was proposed to fix the issue?

Having try{} finally around using clutters the code very much! For me is the nested using much more readable.

Question: Is there a better way of doing things? I am looking for a solution which will be intuitively understandable. Everyone who will see this last snippet will be curios about what is happening.

Thanks in advance for your answers.

Community
  • 1
  • 1
George Mamaladze
  • 7,593
  • 2
  • 36
  • 52
  • Questions like you had in your P.S. probably belong on MetaStackoverflow. In fact, that question may already have been asked and answered there. – Matt Fenwick Nov 11 '11 at 15:58
  • By the way I tried also following code `using (StringReader strReader = new StringReader(xmlString)) using (XmlReader xmlReader = XmlReader.Create(strReader)) { strReader = null; result.ReadXml(xmlReader); }` leads to compile error Error 18 Cannot assign to 'strReader' because it is a 'using variable' – George Mamaladze Nov 11 '11 at 21:20

2 Answers2

21

The problem isn't because of the nested usings. They're fine and generally recommended. The problem here is that XmlReader will dispose the TextReader if you pass an XmlReaderSettings with CloseInput == true, but the CA2202 rule isn't smart enough that your code won't go down that branch. Keep your nested usings, and suppress the CA2202 violation as a false positive.

If you want to be explicit in your code in order to enhance its readability and/or maintainability, use an XmlReaderSettings with CloseInput set to false, but that is the default value, so it's not strictly necessary, and, to be clear, would not satisfy the rule.

BTW, there are similar CA2202 problem scenarios for a variety of stream and reader types. Unfortunately, they're not all the same as this one, so the best case handling can differ depending on which type is cause the problem.

ruffin
  • 16,507
  • 9
  • 88
  • 138
Nicole Calinoiu
  • 20,843
  • 2
  • 44
  • 49
  • `xmlReaderSettings.CloseInput = false` does not satisfy the rule. Suppression really seems to be the only correct way handling this problem. It seems generally to be a good idea to tolerate double disposal in `Dispose` implementations, thus some users like `XmlReader` might dispose you silently and if someone wraps your class in a `using` statement it makes BOOM! Thanks for great explanation. Accepted! – George Mamaladze Nov 11 '11 at 21:54
  • 4
    I wasn't suggesting that setting CloseInput to false would satisfy the rule, but rather that you might want to be explicit in your code in order to enhance its readability and/or maintainability. As for tolerating multiple disposal, there's a .NET design guideline for this, even if there's no corresponding FxCop rule. – Nicole Calinoiu Nov 14 '11 at 14:24
0

I recently had a similar issue, but as I was using a serializer had to adapt it as I wasn't able to set the stringWriter to null right away. This workaround avoids all CA warnings: 

StringWriter stringWriter = null;
XmlWriter xmlWriter = null;
string serializedValue = null;

try
{
    XmlSerializer xmlserializer = new XmlSerializer(typeof(T));
    stringWriter = new StringWriter();

    xmlWriter = XmlWriter.Create(stringWriter);
    xmlserializer.Serialize(xmlWriter, value);
    xmlWriter.Flush();
    serializedValue = stringWriter.ToString();
}
finally
{
    if (xmlWriter != null) //Both objects need disposed 
    {
        xmlWriter.Dispose(); //stringWriter will dispose automatically too
    }
    else if (stringWriter != null) //XmlWriter failed to create
    {
        stringWriter.Dispose(); //just dispose stringWriter
    }
}
Logan A
  • 1