My favorite bugs with IDisposable

System.IDisposable is a foundational interface used in most .NET programs. Its primary purpose is to provide a mechanism for releasing “unmanaged” resources: file streams, database connections, network sockets, etc. Here’s the whole thing:

public interface IDisposable {
    void Dispose();
}

Can’t get much simpler than that!

IDisposable is also one of the few interfaces blessed with special syntax: the using statement. This lets us be very clear about when we clean up resources:

using(var stream = File.OpenText("..."))
{
    // our unmanaged `stream` resource lives
    // for the duration of this block
}
// now `stream` is disposed

Nice and straightforward!

Despite the simplicity of the interface and the special syntax, it is surprisingly easy to write buggy IDisposable code.

Here are a few of my favorites.

IDisposable lies to you

Based on the definition, the interface is very easy to implement, just one void method and we’re done! However, IDisposable is cheating. It has some requirements that can’t be expressed in C#:

• interaction with the garbage collector via finalizers
• interaction with sub-classes
• handling repeated calls to Dispose

IDisposable has a lot of extra-linguistic concerns, and to meet all of those we use the dispose pattern. This is such a common problem that linters and IDEs will all chime in to help us implement IDisposable right.

In this respect, IDisposable is violating some of our fundamental assumptions about the completeness of C# interfaces; it’s not enough to get the compiler happy, we have to read the docs, follow our linters, and satisfy any requirements that the compiler can’t enforce.

This is unfortunately common:

• IEquatable<T>

  If you implement IEquatable<T>, you should also override the base class implementations of Equals(Object) and GetHashCode()…

• IComparable<T>

  If you implement IComparable<T>, you should overload the op_GreaterThan, op_GreaterThanOrEqual…

• IFormattable

  A class that implements IFormattable must support the “G” (general) format…

The base class libraries are littered with these extra-linguistic issues! Be sure to use some analyzers to help out. I’ve had good luck with Roslynator.

IDisposable implementors can lie to you

We know we need to use the dispose pattern when implementing our own IDisposables, but what about when we use other people’s IDisposables? For example, does SqlConnection use the dispose pattern correctly? We don’t have a great way to tell, other than reading the documentation.

The biggest offender I’ve run across here is HttpClient. We usually want to release our unmanaged resources as soon as we can. By implementing IDisposable, HttpClient is sending us a strong signal that it wants to be in a using block:

using(var client = new HttpClient())
{
    // make a request
}

Calling HttpClient this way will mess you up. Tucked away in the documentation we find this gem:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads.

This is a wonderful bug that will work fine for development and manual testing, but blow up when you scale, and it doesn’t take a lot of traffic. I’ve seen this one first hand.

Use IHttpClientFactory to implement resilient HTTP requests has more background and a good fix on this issue.

using issues with yield

IDisposable and using give us tools to control when our resources get cleaned up, but this can conflict with the lazy evaluation we get from the yield keyword.

Let’s assemble a minimal example and use Console.WriteLine to see how the bug works. We’ll need a IDisposable implementation, and a few helper functions to introduce some indirection.

Let’s start with this working code:

// _not_ following dispose pattern
// for brevity
class FakeResource : IDisposable
{
    public void Dispose()
    {
        Console.WriteLine("dispose");
    }
}

static IEnumerable<int> Query(FakeResource r)
{
    Console.WriteLine("query");
    return new[]{ 1 };
}

static IEnumerable<int> Query()
{
    Console.WriteLine("open");
    using(var r = new FakeResource()){
        return Query(r);
    }
}

public static void Main()
{
    Console.WriteLine("start");
    foreach(var i in Query())
    {
        Console.WriteLine($"result: {i}");
    }
    Console.WriteLine("done");
}

When we run this, we get the desired output:

start
open
query
dispose
result: 1
done

All the operations are in the right order, and we cleaned up our resources as soon as we could. Let’s change Query to use yield return instead:

 static IEnumerable<int> Query(FakeResource r)
 {
    Console.WriteLine("query");
-    return new[]{ 1 };
+    yield return 1;
 }

Now we get a very undesirable output:

start
open
dispose
query
result: 1
done

Our query is now happening after we dispose! What gives?

When we switched to yield return, the compiler did a lot more work. It generated a uniquely named class implementing IEnumerable<int>. Our Query(FakeResource) returns an instance of that class, and we don’t run any of our code until the foreach starts looping. Jon Skeet has a great breakdown of the gory details in Iterator block implementation details: auto-generated state machines.

The big difference here is we went from eager to lazy evaluation, and that changed our effective order-of-operations; we left the using block before running our code in Query(FakeResource)

This bug is great because the yield return change and the using can be miles away from each other. We need at least one level of indirection for this bug to work, and that’s pretty common. The yield return change looks innocent on its own, and the compiler is happy the whole time.

This kind of bug can also be created with async and await; we can leave the using block while a task still wants to use our IDisposable.

Over-eager Disposes

In many cases we end up passing an IDisposable around between methods or into constructors. This brings up a question: am I supposed to Dispose this, or is someone else going to? When we call Dispose from the wrong spot, we get subtle, hard to diagnose bugs.

A guideline that has served me well: If you called new, it’s on you to call Dispose. If you didn’t call new, it’s not your responsibility to call Dispose.

Consider this function:

void Save(IDbConnection connection)
{
    using(connection)
    {
        // do some work with connection
    }
}

The developer is trying to do the right thing and clean up when they are done, but now calling Save has a side effect of closing the connection. This can create errors elsewhere:

using(var conn = new SqlConnection("..."))
{
    Save(conn);
    Update(conn); // *boom*
}

We’ll get an exception thrown from Update because the Save function closed our connection. This bug can be pretty sneaky; the exception will get thrown from Update, so the investigation will start in the wrong place. Depending on the codebase and levels of indirection it might take a lot of time to discover that Update only fails when called after Save. This bug can sit dormant in a codebase for years, and show itself when someone adds the call to Update.

This bug can also be expressed in an objected-oriented fashion, and the debugging get murkier when we get our IDisposable via dependency injection (DI).

Consider this class:

// _not_ following dispose pattern
// for brevity
public class Saver : IDisposable
{
    readonly IDbConnection _conn;

    public Saver(IDbConnection conn)
    {
        _conn = conn;
    }

    public void Save()
    {
        // do some work with _conn
    }

    public void Dispose()
    {
        _conn.Dispose();
    }
}

Again, the developer is trying to do the right thing and clean up when they are done, but we’re introducing unexpected side effects on the IDbConnection.

When working with a dependency injection library, we can choose how long different types should live, and it’s very easy to get this effective code:

using(var conn = new SqlConnection("..."))
{
    using(var saver = new Saver(conn))
    {
        saver.Save();
    }
    using(var updater = new Updater(conn))
    {
        updater.Update(); // *boom*
    }
}

Again, the stack trace will lead us to Update, but likely be muddled with calls through DI. Depending on the codebase and DI config, finding the relationship between Save and Update could be nearly impossible.

In addition to the misleading exceptions, what really makes this bug great is it happens when we’re making extra effort trying to do the right thing. My gut reaction is to get bitter and passive aggressive towards my program.

I’m sooo sorry I tried to clean up my unmanaged resources the way you told me to do. I’ll never do it again.

Conclusion

IDisposable looks easy on the surface, but, like most things in life, contain multitudes.

We can protect ourselves and our systems by:

• reading the docs more than we think we should have to
• hooking up linters to watch for things the compiler can’t catch
• following an IDisposable policy where the component that called new has the sole responsibility to call Dispose