Reputation: 6175
The Dispose
pattern is notoriously complicated to get right, especially when we have a hierarchy of classes that needs to dispose things at different levels. The recommended implementation is as follow, took from Implement a Dispose method - Microsoft Docs.
using System;
class BaseClass : IDisposable
{
// To detect redundant calls
private bool _disposed = false;
~BaseClass() => Dispose(false);
// Public implementation of Dispose pattern callable by consumers.
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
// Protected implementation of Dispose pattern.
protected virtual void Dispose(bool disposing)
{
if (_disposed)
{
return;
}
if (disposing)
{
// TODO: dispose managed state (managed objects).
}
// TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
// TODO: set large fields to null.
_disposed = true;
}
}
using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;
class DerivedClass : BaseClass
{
// To detect redundant calls
private bool _disposed = false;
// Instantiate a SafeHandle instance.
private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);
// Protected implementation of Dispose pattern.
protected override void Dispose(bool disposing)
{
if (_disposed)
{
return;
}
if (disposing)
{
// Dispose managed state (managed objects).
_safeHandle?.Dispose();
}
_disposed = true;
// Call base class implementation.
base.Dispose(disposing);
}
}
What I don't get in this implementation is what advantage we have in adding a _disposed
field at each level of the hierarchy? Instead, we could take care of the _disposed
only in the top level of the hierarchy (the one implementing directly IDisposable
and not care about it in the derived classes.
Something like this:
using System;
class BaseClass : IDisposable
{
// To detect redundant calls
private bool _disposed = false;
~BaseClass()
{
if (_disposed)
{
return;
}
Dispose(false);
_disposed = true;
}
// Public implementation of Dispose pattern callable by consumers.
public void Dispose()
{
if (_disposed)
{
return;
}
Dispose(true);
GC.SuppressFinalize(this);
_disposed = true;
}
// Protected implementation of Dispose pattern.
protected virtual void Dispose(bool disposing)
{
if (disposing)
{
// TODO: dispose managed state (managed objects).
}
// TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
// TODO: set large fields to null.
}
}
using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;
class DerivedClass : BaseClass
{
// Instantiate a SafeHandle instance.
private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);
// Protected implementation of Dispose pattern.
protected override void Dispose(bool disposing)
{
if (disposing)
{
// Dispose managed state (managed objects).
_safeHandle?.Dispose();
}
// Call base class implementation.
base.Dispose(disposing);
}
}
This is such a widely used code sample that there must certainly be a good reason for it to be implemented with the repeated _disposed
at each level, but I just can't find any.
It's a tiny bit more code in the base class, but less to worry about in the derived class, and some repeated information removed.
What else am I missing?
Edit:
As @InBetween correctly says that one drawback of my implementation is that if you'd need to check if your object is disposed in one method of the derived class you won't be able to check it. Let's correct that issue by making it a protected property with a private set.
using System;
class BaseClass : IDisposable
{
// To detect redundant calls
protected bool Disposed { get; private set; } = false;
~BaseClass()
{
if (Disposed)
{
return;
}
Dispose(false);
Disposed = true;
}
// Public implementation of Dispose pattern callable by consumers.
public void Dispose()
{
if (Disposed)
{
return;
}
Dispose(true);
GC.SuppressFinalize(this);
Disposed = true;
}
// Protected implementation of Dispose pattern.
protected virtual void Dispose(bool disposing)
{
if (disposing)
{
// TODO: dispose managed state (managed objects).
}
// TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
// TODO: set large fields to null.
}
}
using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;
class DerivedClass : BaseClass
{
// Instantiate a SafeHandle instance.
private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);
public void DoSomething()
{
if(Disposed)
{
throw new ObjectDisposedException("Cannot access disposed resource");
}
}
// Protected implementation of Dispose pattern.
protected override void Dispose(bool disposing)
{
if (disposing)
{
// Dispose managed state (managed objects).
_safeHandle?.Dispose();
}
// Call base class implementation.
base.Dispose(disposing);
}
}
Upvotes: 4
Views: 625
Reputation: 52290
If you are inheriting from a disposable class, one of two conditions must be true.
Your subcless does not introduce a new disposable resource. In this case you don't need to override Dispose
and the question is moot.
Your subclass introduces a new disposable resource. In this case, you're going to override Dispose
, insert your own disposal code, then call base.Dispose
. The _disposed
flag is there to help you remember to prevent your disposal code from executing twice.
You certainly can remove _disposed
if you want. But you probably do not care much about the base class' _disposed
flag, if it even has one. It worries about what it is disposing, and you worry about yours.
Upvotes: 4
Reputation: 32780
The reason is simple. In your implementation you cant use _disposed
in the derived type to check if any method is invoked when the object is already disposed and take the necessary actions. In your implementation you'd need to create your own redundant flag isDisposed
which defeats the purpose; you already have one "for free" from the pattern itself following the guidelines.
A case could be made though about making _disposed
protected
.
Upvotes: 3