Is Microsoft IDisposable pattern actually correct?-CodePudding

I've stumbled across Microsoft's recommended way to implement the IDisposable pattern many times, it's even present in Visual Studio as an "Implement Interface" option in the lamp icon menu. It looks like this:

// Override only if 'Dispose(bool disposing)' has code to free unmanaged resources
~Foo() {
    // Do not change this code.
    Dispose(calledByFinalizer: true);
}

public void Dispose() {
    // Do not change this code. 
    Dispose(calledByFinalizer: false);
    GC.SuppressFinalize(this);
}

// Put cleanup code here
protected virtual void Dispose(bool calledByFinalizer) {
    if (_disposed) return;

    if (!calledByFinalizer) { /* dispose managed objects */ }

    /* free unmanaged resources and set large fields to null */

    _disposed = true;
}

I refactored the suggested code a bit (because Dispose(bool disposing) can break someone's brain, and nested if's can break someone's eyes).

But I still have some questions on my mind:

It is assumed that the method will be called once. Then why is _disposed = true placed at the end of the method and not at the beginning? If IDisposable.Dispose() is called from different threads, then they can all bypass the if (_disposed) return; check and actually execute the method body twice. Why not do it like this:

    if (_disposed) return;
    else _disposed = true;

Why is protected virtual void Dispose(bool disposing) flagged as virtual? Any derived class does not have access to the _disposed field and can easily break its behavior. We can only mark as virtual the optional part where the derived class can do anything without calling base.Dispose():

~Foo() => FreeUnmanagedResources();

public void Dispose() {
    if (_disposed) return;
    else _disposed = true;

    DisposeManagedObjects();
    FreeUnmanagedResources();

    GC.SuppressFinalize(this);
}

protected virtual void DisposeManagedObjects() { }
protected virtual void FreeUnmanagedResources() { }

CodePudding user response：

The pattern is correct, but assumes the worst case scenario, where you must also implement a finalizer. That is, if you need a finalizer you must also follow the entire pattern. However...

You don't usually need a finalizer at all.

You only need a finalizer if you are creating the original managed wrapper for an unmanaged resource.

For example, if you have created a brand new, never before seen database system, where connections to the database are an unmanaged resource, then your managed ADO.Net DbConnection wrapper must implement a finalizer. But if you creating a wrapper object for your application to manage connections to an existing database type, like SqlConnection, OleDbConnection, MySqlConnection, etc, then you should still implement IDisposable but you do not need a finalizer.

And it turns out, when you don't have a finalizer you can safely remove a lot of the code from the documented IDisposable pattern.

CodePudding user response：

You can't assume that Dispose is only going to get called once. In best practice, yes. In the worst practice, not at all. Every situation cannot conveniently use a using statement. So rather than risk the code trying to clean up unmanaged resources twice -- which could go really bad, depending on the type of resource -- there's a flag added that prevents it. This takes a burden off the calling code as far as remembering if dispose has already been called.
Dispose has to be declared as virtual to support any separate cleanup that might need to occur if subclasses are created that instantiate any unmanaged resources that are distinctly different than those used in the base class. Dispose in the subclass should call base.Dispose(); before or after it cleans up its own mess.

CodePudding user response：

The guidance is correct, is accepted best practice and is fully documented here:

The best practice for this pattern is this, with the exception that I personally prefer to wrap this in a region block:

public class T : IDisposable
{
    #region IDisposable

    private bool disposedValue;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                // TODO: dispose managed state (managed objects)
            }

            // TODO: free unmanaged resources (unmanaged objects) and override finalizer
            // TODO: set large fields to null
            disposedValue = true;
        }
    }

    // // TODO: override finalizer only if 'Dispose(bool disposing)' has code to free unmanaged resources
    // ~T()
    // {
    //     // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
    //     Dispose(disposing: false);
    // }

    public void Dispose()
    {
        // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
        Dispose(disposing: true);
        GC.SuppressFinalize(this);
    }

    #endregion IDisposable
}

It is assumed that the method will be called once.

Yes we all make this assumption, but due to programmer discretion, threading and general bad design patterns, we all assume that some nufty will break our rules. Often it is an aggressive form of direct calling dispose in a UI that fires on close, but it happens in many other scenarios too. So the pattern provided is strongly advised to cater for the scenarios where dispose IS called multiple times.

Why is protected virtual void Dispose(bool disposing) flagged as virtual? Any derived class does not have access to the _disposed field...

So the reason this method exists AND is virtual is specifically because the inheriting class does not have access to the _disposed field, the overriding implementation should call the base implementation:

base.Dispose(disposing):

If they don't then they will break the intended functionality as you say, but that is their perrogative, there are valid cases where we do specifically want to change the implementation, that is why we override.

As a UI control author this pattern is very useful and is a good level of encapsulation. In cases like yours most will end up overriding both DisposeManagedObjects and FreeUnmanagedResources. By separating these concerns you make it too hard to do any other C# Kung Fu that we might also want to do.

Your implementation is not bad, and in your use case arguably safer, but it is uniquely yours and does make some advanced tasks harder to implement or maintain because now the disposal code is split across 2 methods.

By using standard patterns, other developers and development tools are more likely to intrinsically understand and interact with your classes. This pattern has been developed over a number of years and is an aggregate of how we all used to do things in our own quirky ways.

The dispose pattern is hard to understand and until you need to override it, it is hard to see the value in this implementation. But when it goes wrong it is incredibly hard to debug, because it is often the last place we look (we always assume that it works) but it is also hard to track outside of a using(){} block.

Use the standards, those who inherit your code or run maintenance on it later will thank you.