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:
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.
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:
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.
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
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.
StringReader is not the best example of the case of “extremely important” disposing. Its Dispose does nothing.
Hi David,
I came across the below code block sometime back, not sure if using is really helping here –
YES, use using hear. StreamReader will cause a leak.
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;
}
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.
I have not found any tool that finds everything. That’s why it still comes down to checking manually to find everything. I will be revisiting this for Part 3.
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.
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.