Is this implementation of IDisposable correct?
I can't remember all the rules for implementing the IDisposable interface, so I tried to create a base class that takes care of all of this and makes it easier to implement IDisposable. I just wanted to hear your feedback if this implementation is ok, or if you see something I can improve. The user of this base class is expected to extract from it and then implement the two abstract methods ReleaseManagedResources()
and ReleaseUnmanagedResources()
. So here's the code:
public abstract class Disposable : IDisposable
{
private bool _isDisposed;
private readonly object _disposeLock = new object();
/// <summary>
/// called by users of this class to free managed and unmanaged resources
/// </summary>
public void Dispose() {
DisposeManagedAndUnmanagedResources();
}
/// <summary>
/// finalizer is called by garbage collector to free unmanaged resources
/// </summary>
~Disposable() { //finalizer of derived class will automatically call it base finalizer
DisposeUnmanagedResources();
}
private void DisposeManagedAndUnmanagedResources() {
lock (_disposeLock) //make thread-safe
if (!_isDisposed) { //make sure only called once
try { //suppress exceptions
ReleaseManagedResources();
ReleaseUnmanagedResources();
}
finally {
GC.SuppressFinalize(this); //remove from finalization queue since cleaup already done, so it not necessary the garbage collector to call Finalize() anymore
_isDisposed = true;
}
}
}
private void DisposeUnmanagedResources() {
lock (_disposeLock) //make thread-safe since at least the finalizer runs in a different thread
if (!_isDisposed) { //make sure only called once
try { //suppress exceptions
ReleaseUnmanagedResources();
}
finally {
_isDisposed = true;
}
}
}
protected abstract void ReleaseManagedResources();
protected abstract void ReleaseUnmanagedResources();
}
You access the _disposeLock managed object in the finalizer. By then, the trash may have already been collected. Not sure what the implications will be as you only use it for blocking.
Thread-safety seems like overkill. I don't think you need to worry about contention between the GC thread and the application thread.
Otherwise it looks right.
source to share
I cannot comment on the correctness of your code, but I doubt if you find that the Disposable class at the bottom of your inheritance tree is as flexible as necessary. It won't be very useful if you want to inherit what you don't have. I think IDispoasble has been abandoned as an interface because of the many different domains it can be used through. Providing a concrete implementation at the bottom of the inheritance hierarchy would be ... a limitation.
source to share
EDIT Yes, I am misreading the part about separating managed and unmanaged resources (still on my first cup of coffee).
However, blocking is still almost unnecessary. Yes, during passive disposition, the code will run on the finalizer thread, which is different from the thread it was originally executed from. However, if an object terminates in this way, the CLR has already determined that there are no existing references to that object and therefore collects it. Thus, there is no other place that can trigger your order at that time, and therefore no reason to block.
Several other style comments.
Why abstract methods? By doing so, you are forcing derived classes to inject methods to manage managed and unmanaged resources, even if they don't have any to dispose of. True, there is no point in extracting from this class if you have nothing to dispose of. But I think this is quite common for one or the other, but not both. I would make them virtual or abstract.
Also, you prevent double deletion, but you do nothing to warn the developer that they are deleting the object twice. I understand that the MSDN documentation says that double disposition should be essentially non-operational, but at the same time, under what circumstances should it be legal? In general, it is a bad idea to access the object after deleting it. Double disposal requires that the object to be disposed be reused and is likely a bug (this can happen if finalization is not suppressed at active disposal, but this is also a bad idea). If I were to implement this, I would throw a double directive to warn the developer that they misused the object (which is using it after it has already been deleted).
source to share
If you are concerned about thread safety, I would use lightweight blocking operations rather than a heavyweight lock:
class MyClass: IDispose {
int disposed = 0;
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
public virtual void Dispose(bool disposing)
{
if (0 == Thread.InterlockedCompareExchange(ref disposed, 1, 0))
{
if (disposing)
{
// release managed resources here
}
// release unmanaged resources here
}
}
~MyClass()
{
Dispose(false);
}
}
source to share