C# Singleton – Using IDisposable Without Finalizer

csingleton

Is it a right expectation that if a C# class deals with unmanaged resources and implements IDisposable, then it also should implement some kind of finalizer logic?

We have a vendor-supplied utility class that uses some unmanaged resources. We don't have the source code, so don't know too much implementation details, apart from the fact that it implements IDisposable but does not have any finalizer or SafeHandle. So without calling Dispose, it leaks. Simplified sample code:

public class Utility : IDisposable
{
    //This class has some unmanaged resources
    ...
    //Utility function
    public void DoSomething(...) { ... }
    //IDisposable
    public void Dispose() { ... }
    protected virtual void Dispose(bool disposing) { ... }
}

In our code we created a facade around it to simplify the usage and also added a singleton as it is used at dozens of places in our complex legacy codebase. Example:

public class UtilityFacade : IDisposable
{
    protected Utility utility = new Utility();
    //Simplified utility function
    public void DoSomethingGood() { utility.DoSomething(1,2,3); }
    //IDisposable
    protected bool disposed = false;
    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                utility.Dispose();
            }
            disposed = true;
        }
    }
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    //Singleton
    public static UtilityFacade Instance { get; } = new UtilityFacade();
}

So from our applications (web site, web service, console apps) it would be used like this:

UtilityFacade.Instance.DoSomethingGood();

The problem is that since the Instance is a static member, it will never be disposed via IDisposable, so the Utility class will not be disposed either, and since it doesn't implement any finalizer, it leaks unmanaged resources at the end of the application process.

Which is the right choice here:

  1. The vendor says we should call Utility.Dispose() when the application finishes. This would require extra coding for us, e.g. hooking up to Application_End for web apps, or adding try{}finally{…} blocks to console apps for deterministic cleanup, or adding a finalizer to the facade class (which would keep the code related to Utility at one place at least).
  2. We request the vendor to implement the finalizer.
    Since the Utility class is dealing directly with the unmanaged resources, they should add the finalizer logic. According to some articles on the net it is the best practice recommended by Microsoft.

Or any other recommendation?

Best Answer

A finalizer should be implemented for any class that manages unmanaged resources; the vendor's class would be improved if they did so. However, this won't necessarily improve your code, because finalizers are not guaranteed to be called, and in fact finalizers of objects that are reachable through statics are very unlikely to be called.

So the right solution is to call Dispose on your facade at the right time. The best way to achieve this is to not make it a singleton in the first place. Inject the instance into the components that need it, and make sure that the instance is cleaned up correctly. For console apps, this may mean a simple using block in Main. For web apps, you should probably have a DI container already (especially if you're using ASP.Net Core) which is wired to be disposed at application shutdown, and will itself dispose all the non-transient instances it manages.

Related Topic