Hidden IDisposable Issues in Microsoft .NET

As an experienced .NET software engineer, whenever I am hired into a new team, I almost immediately get tasked to fix coding issues. I guess since I am dotNetDave and I wrote the only coding standards book for .NET! Out of all the millions of lines of code I have reviewed in the past 20 years of .NET, hands down the #1 issue are developers not properly disposing of objects that implement IDisposable. In some teams, it has taken me many months of hard work to fix these issues that will always lead to virtual memory leaks.

While working on a solution recently, I Tweeted the following:

Hidden Dispose Issues-Tweet

This post is to answer Evil Mikes question. If you have a coding question for me, please email me using this page: https://dotnettips.wordpress.com/about/.

The Problem

The problem I am seeing is that I see a lot of code like this:

public static TResult Deserialize<TResult>(string xml) where TResult : class
{
    var xs = new XmlSerializer(typeof(TResult));

    return (TResult)xs.Deserialize(new StringReader(xml));
}

The issue with this is the final line where a new StringReader is created. StringReader (and just about every type that ends in ‘Reader’) is one of those types that implement IDisposable to clean up the memory it uses. Since it does, it’s extremely important that the developer call .Dispose when it’s no longer needed or use one of the using code blocks.

I can prove that Dispose is not being called by looking at the IL for this method.

Hidden Dispose Issues-1

As you can see, there isn’t a call to Dispose in the IL.

The Solution

The following code, from my Spargine open-source project, properly codes this method as shown below:

public static TResult Deserialize<TResult>(string xml) where TResult : class
{
    using var sr = new StringReader(xml);
    var xs = new XmlSerializer(typeof(TResult));

    return (TResult)xs.Deserialize(sr);
}

As you can see, this uses the new way of implementing the using code statement. This means that at the end of the code block, in this case the method, Dispose will be called on StringReader, and its memory will be cleaned up. Want proof? Here it is:

Hidden Dispose Issues-2

As you can see, a try/finally is created by the compiler and in the finally Dispose is being called. This should be the only way to code types that implement IDisposable and not the way that was first shown.

Summary

Since these issues have been around ever since .NET was created, I wished Visual Studio would alert the coder that they are causing a memory leak! Until then, be careful or hire me to help your team!

Thanks again to Evil Mike for the question, keep them coming! Mike, if you live in the U.S., contact me if you want me to send you some swag from dotNetTips.com, DevExpress.com and a copy of my book!

If you have any comments or suggestions, please comment below.

Entertaining the geeks in Mauritius!

Don’t forget to learn all about code performance for Microsoft .NET by going to the Code & App Performance Page: https://bit.ly/CodeAppPerformance.
I hope you will support this site by buying dotNetDave a cup of coffee: https://www.buymeacoffee.com/dotnetdave

9 thoughts on “Hidden IDisposable Issues in Microsoft .NET

  1. IDisposable is probably the most abused interface in the entire eco system. Direct from the documentation: “Provides a mechanism for releasing unmanaged resources.” If you r class does not have a member which contains something that holds on to an unmanaged resource (directly or indirectly) your class has no business implementing IDisposable….

    Consider the overhead (knowing about Finalizers, ensure Dispose can be called multiple times, accssing a member must throw ObjectDisposedException, any class that has a member of your type must also implement IDisposable (and take on all thos constraints.

    Remember [Dave, I know you know this].. asside from the syntactic sugar of the using statement, the .Net runtime will NEVER implicitly call the Dispose method.

    If you are not holding an unmanaged resource, do yourself (And every consumer of your code) a big favor and use one of the many other design patterns to manage operations as an item goes out of scope.

  2. StringReader is not the best example of the case of “extremely important” disposing. Its Dispose does nothing.

  3. Hi David,
    I came across the below code block sometime back, not sure if using is really helping here –

    string emailBody = string.Empty;
    StreamReader reader = null;
            try
            {
                using (reader = new StreamReader(path))
                {
                    emailBody = reader.ReadToEnd();
                }
            }
            catch 
            {
                throw ; 
            }
            reader.Close();
    
  4. Over the years, I have experimented with a pattern I call MustDispose. It’s an object which throws an exception if it’s ever garbage collected without first being disposed. Here’s an old version.

    // Copyright © 2003 by Martin L. Shoemaker
    // All rights reserved.

    using System;
    using System.Diagnostics;

    namespace Disposable
    {
    ///
    /// Include a member of class MustDispose in a class for which you
    /// want to require that Dispose be called.
    ///
    ///
    /// This object will either throw an exception or assert if it
    /// is destroyed without being disposed first. If you include it in
    /// your class, and call its Dispose in your Dispose, then your
    /// class gets a “standard” implementation of mandatory disposal.
    ///
    /// This object also serves as a flag for whether Dispose has been
    /// called or not. IDisposable rules require that redundant calls
    /// to Dispose are harmless.
    ///
    /// Example usage:
    ///
    /// MustDispose m_Dispose = new MustDispose( "MyClass" , true );
    ///
    /// public void Dispose()
    /// {
    /// if ( ! m_Dispose.Disposed )
    /// {
    /// // TODO: Clean up resources.
    ///
    /// // We cleaned up. Record that.
    /// m_Dispose.Dispose();
    /// }
    /// }
    ///

    ///
    public class MustDispose : IDisposable
    {
    private string m_ClassName;
    private bool m_Throw = false;
    private bool m_Disposed = false;

        /// <summary>
        /// Simple constructor. Exceptions will not be thrown.
        /// Assertions will fire in debug mode.
        /// </summary>
        /// <param name="ClassName">
        /// Name of the containing class.
        /// </param>
        public MustDispose(string ClassName)
        {
            m_ClassName = ClassName;
        }
    
        /// <summary>
        /// Constructor. Choose whether to throw exceptions in
        /// both debug and release mode, or only fire assertions
        /// in debug mode.
        /// </summary>
        /// <param name="ClassName">
        /// Name of the containing class.
        /// </param>
        /// <param name="Throw">
        /// Do we throw (true) or only assert (false)?
        /// </param>
        public MustDispose(string ClassName,bool Throw)
        {
            m_ClassName = ClassName;
            m_Throw = Throw;
        }
    
        ~MustDispose()
        {
            string ErrMessage = "Class " + m_ClassName +
                " has clean-up code that requires a 'using' clause or a Dispose call.";
            if ( ( m_Throw ) &&
                 ( ! m_Disposed ) )
            {
                throw new Exception( ErrMessage );
            }
            Debug.Assert( m_Disposed , "Did not dispose this!" ,
                ErrMessage );
        }
    
        /// <summary>
        /// Clean up resources.
        /// </summary>
        public void Dispose()
        {
            m_Disposed = true;
        }
    
        /// <summary>
        /// Have we disposed already?
        /// </summary>
        public bool Disposed
        {
            get
            {
                return m_Disposed;
            }
        }
    }
    

    }

  5. Is this something that can be picked up by static analysis tools? I imagine in a sprawling legacy codebase there could be hundreds of instances of code like this.

  6. Hi David,

    While browsing through the reference implementation for StringReader.cs I noticed that the Dispose() method simply sets some class attributes to their default values, i.e. there’s no unmanaged resource releasing happening (see https://referencesource.microsoft.com/#mscorlib/system/io/stringreader.cs).

    I think in your example there’s no potential for a memory leak because the StringReader doesn’t own unmanaged resources, and would be GCed anyways if it goes out of scope.

    1. You are correct. I treat all types that implement IDisposabe as a potential issue. I don’t have time to inspect each type and its base types. So I recommend just coding any IDispoable type the same. Plus we have no idea if they will add things in the future that could cause virtual memory leaks.

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.