.NET Disposable Pattern Guidelines - How Good Are They?

After having to look into the recommended way of implementing the Disposable pattern in .NET/C#, there's a lot about it I find non-optimal. Ranging from the philosophical to actual implementation decisions.

.NET Disposable Pattern Guidelines - How Good Are They?

I started getting exposure to C# in my current job after a really long time. The last time I recall using it was over 10 years ago while still in Argentina.

One of the the first things I stumbled upon was the Disposable pattern.
The Disposable pattern is the recommended way by Microsoft to handle unmanaged resources in the .NET platform languages.
All the information about how to implement it, recommendations, etc can be found in Microsoft's documentation here, here and here.

The gist of it is straight-forward. It is a deterministic way to provide resource cleanup once the object is not going to be used anymore.
Coming from C++, this would be a rough equivalent as what one would do in a destructor when using RAII.

But there are so many things that I find hilariously wrong about the recommended approach in .NET at so many different levels that I have a hard time knowing where to start.

Philosophical Concern: Asked to Tolerate Contract Breaching

The contract between a client and Disposable object is simple: once the object is not used anymore call its Dispose() method.
The C# language provides mechanisms to guarantee proper usage of the pattern: the using statement will dispose of an object once it goes out of scope; and code analysis rule CA1001 will complain if a non-disposable class has a disposable member.

But in the guidelines, Microsoft asks us to work around clients not following the contract:

Dispose

WHY?!?!? The contract is clear, the consequences of not following the contract are clear: it is a bug on the client to not follow the contract!
Software should not work around broken contracts, the behavior should be listed as undefined and let incorrect programs fail.

Contracts are defined for a reason: to understand how an object should behave and under what circumstances that object will behave as expected; anything that doesn't fall within the contract cannot be trusted to work in any particular way.
They are crucial to determine the responsible party when a bug arises: if the client is working within the contract's boundaries then it is the implementer that has a bug, otherwise responsibility relies on the client.
Lack of clear, unambiguous contracts is a common source of bugs in software and tolerating behavior outside the contract definition adds to the ambiguity.

Providing workarounds for contract-breaching behavior is in and of itself a change in the contract definition. It adds ambiguity and allowing incorrect uses to "work just fine" can and will probably backfire to clients down the road. And it will likely break in ways the client maintainer will find hard to understand.

Design Concern: Object is Responsible to Track its Liveness

In the guidelines, it is requested that the Dispose() method needs to be idempotent and allowed to be called multiple times:

To help ensure that resources are always cleaned up appropriately, a Dispose method should be idempotent, such that it is callable multiple times without throwing an exception. Furthermore, subsequent invocations of Dispose should do nothing.

In all the guidelines it is advised (and assumed) that the object will track whether it has already been disposed of or not:

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

If we think what this method's purpose is, it is mind-boggling that calling it multiple times should be allowed, even less expected and tolerated.
This method is supposed to function as a pseudo-destructor, that needs to be called once the object is not needed anymore, thus translates to "this is the final method to be called". Why should this consider handling being called multiple times?

The fact that the object needs to track whether it is still alive or not is plain wrong. It adds complexity when there shouldn't need to be. By contract, the Dispose() method is the last method that needs to be called and any other call to the object is undefined behavior.
Following the same logic used here, all method in the class should actually be checking for the liveness flag and do nothing if the object is not alive: I doubt this would pass any serious PR review.

Design Concern: Disposable Inheritance Management

I'm gonna be honest here, reading how the guidelines explained how to manage the Disposable pattern when inheritance is involved made me want to cry.

To summarize, the way .NET guides you to implement it is to (1) implement the interface as a non-overridable Dispose() method in the base class, (2) add a virtual Dispose(bool isDisposing) method which will do the actual clean up and (3) call the virtual method from the non-overridable one.
The boolean flag is used to state whether the method is being called from the Dispose() method or from the finalizer (safeguard protecting the breach of contract!).

There are no reasons given as to why this approach is the standard way of handling the Disposable pattern implementation for inherited classes.
My best guess after reading the documentation is that this is needed to accommodate the breach of contract safeguard in a safe manner: the flag is used to determine whether to forward the Dispose() method to disposable managed resources or not.

There are several smells in this approach. First of all, the usage of a boolean flag to define behavior. It would have been better to just define two methods: DeterministicDispose() and NonDeterministicDispose() to keep proper separation and let implementors decide how to implement each.
Second, the fact that this level of complexity needs to be added to handle the case when the behavior doesn't follow the expectations is not proper. If the contract is expected to be followed for things to work correctly, an overridable Dispose() with the need to call the base class' Dispose() would have sufficed.
Lastly, that the behavior needs to be different based on where the call is coming from points to a design error as methods should be as self-contained as possible and not depend on the caller's context.

Implementation Concern: Garbage Collector Behavior Manipulation

The documentation advises to call GC.SuppressFinalize(this) from the Dispose() method. This prevents the garbage collector from calling the finalizer in case one exists. The reason given is that all the cleanup should be done by the Dispose() method.

The fact that the language recommends changing underlying infrastructure from business-level logic doesn't seem right.
Personally, I think this is more likely related to the fact that they recommend using the finalizer as the safeguard for users not following the contract:

Define a finalizer. Finalization enables the non-deterministic release of unmanaged resources when the consumer of a type fails to call IDisposable.Dispose to dispose of them deterministically.

Recommending general-purpose patterns to manipulate basic runtime component's behavior should be a red-flag. The less knowledge business or domain level code needs about the underpinnings of the language and runtime, the better.
It adds complexity and hampers readability and maintainability in the long run.

Conclusion: A Deteriorated View on the Language

Seeing this kind of advise for such fundamental feature coming from the maintainers of the language puts a dent in the trust I have in the language's general coherence and internal design.

I know I don't have a lot of context that might be necessary to understand why it needs to be done this way. But from a newcomer to the language, the whole Disposable pattern implementation (not the concept) was a bit shocking.

Independently from the philosophical differences that I find, this is a feature that is supported natively by the language as it provides mechanisms to properly use it. As such, a lot of the recommendations given in the docs should be done behind the curtains and not be delegated to the implementer.

This is my first big concern from the language, but haven't gotten too deep yet. We'll see what other pros or cons I find as I get more involved in .NET/C# development.

Stay tuned for more!

Comments powered by Talkyard.