I have a class which consumes some unmanaged resources and I would like to free them deterministically and request that the finalizer not be called for the object at hand. My Dispose() method of the class accomplishes this.
In the event of an exception being thrown or something otherwise going wrong or behaving unexpectedly in the constructor, I would like to call Dispose() before throwing. However, I rarely come across implementations which catch thrown exceptions or handle an error in the constructor of disposable objects and then invoke Dispose() on the object - In a lot of cases the author leaves the cleanup to the finalizer. I have not read anything which states that calling Dispose() in a failed constructor is bad practice, but while looking through the .NET source code I have yet to come across such exception or error handling in a disposable object constructor.
Can I call Dispose() inside a "failed" constructor and still be considered a good coding citizen?
Edit to clarify - I am talking about inside the constructor:
public class MyClass : IDisposable
{
private IntPtr _libPtr = IntPtr.Zero;
public MyClass(string dllPath)
{
_libPtr = NativeMethods.LoadLibrary(dllPath);
if (_libPtr != IntPtr.Zero)
{
IntPtr fxnPtr = NativeMethods.GetProcAddress(_libPtr, "MyFunction");
if (fxnPtr == IntPtr.Zero)
{
Dispose(); // Cleanup resources - NativeMethods.FreeLibrary(_libPtr);
throw new NullReferenceException("Error linking library.");
}
}
else
{
throw new DllNotFoundException("Something helpful");
}
}
// ...
}
I wouldn't have an object call Dispose on itself, but I certainly would have a constructor clean itself up if necessary. I'd also like to make that clean-up as simple as possible Considering your example, I'd prefer to compose it something like:
internal sealed class Library : IDisposable
{
IntPtr _libPtr; // Or better yet, can we use or derive from SafeHandle?
public Library(string dllPath)
{
_libPtr = NativeMethods.LoadLibrary(dllPath);
if(_libPtr == IntPtr.Zero)
{
GC.SuppressFinalize(this);
throw new DllNotFoundException("Library Load Failed");
}
}
private void Release()
{
if(_libPtr != IntPtr.Zero)
NativeMethods.FreeLibrary(_libPtr);
_libPtr = IntPtr.Zero; // avoid double free even if a caller double-disposes.
}
public void Dispose()
{
Release();
GC.SuppressFinalize(this);
}
~Library()
{
Release();
}
public IntPtr GetProcAddress(string functionName)
{
if(_libPtr == IntPtr.Zero)
throw new ObjectDisposedException();
IntPtr funcPtr = NativeMethods.GetProcAddress(_libPtr, functionName);
if(_funcPtr == IntPtr.Zero)
throw new Exception("Error binding function.");
return _funcPtr;
}
}
So far nice and simple. Either this object is constructed successfully and can be released by the code that called it, or it doesn't need clean-up. We can even prevent the no-op finalisation, just to be nice. The main thing is that there's nothing that needs cleaned up created after the last thing that could reasonably go wrong.
And then:
public sealed class MyClass : IDisposable
{
private readonly Library _lib;
private readonly IntPtr _funcPtr;
public MyClass(string dllPath)
{
_lib = new Library(dllPath); // If this fails, we throw here, and we don't need clean-up.
try
{
_funcPtr = _libPtr.GetProcAddress("MyFunction");
}
catch
{
// To be here, _lib must be valid, but we've failed over-all.
_lib.Dispose();
throw;
}
}
public void Dispose()
{
_lib.Dispose();
}
// No finaliser needed, because no unmanaged resources needing finalisation are directly held.
}
Again, I can ensure clean-up, but I don't call this.Dispose(); While this.Dispose() could do the same trick, I mainly prefer to have the field I am cleaning up explicit in the same method (the constructor here) that set it but failed to do all of its job. For one thing, the only place there can be a partially-constructed object is in the constructor, so the only place I need be considering a partially-constructed object is in the constructor; I've made it an invariant of the rest of the class that _lib isn't null.
Let's imagine that functions had to be released separately from libraries, just to have a more complicated example. Then I would also wrap _funcPtr to keep with the simplifying rule; either a class has a single unmanaged resource that it cleans up through Dispose() and a finaliser, or it has one or more IDisposable fields that it cleans up through Dispose or it doesn't need disposal, but never a combination of the above.
internal sealed class Function : IDisposable
{
IntPtr _funcPtr; // Again better yet, can we use or derive from SafeHandle?
public Function(Lib library, string functionName)
{
_funcPtr = library.GetProcAddress(functionName);
if(_funcPtr == IntPtr.Zero)
{
GC.SuppressFinalize(this);
throw new Exception("Error binding function.");
}
}
private void Release()
{
if(_funcPtr != IntPtr.Zero)
NativeMethods.HypotheticalForgetProcAddressMethod(_funcPtr);
_funcPtr = IntPtr.Zero; // avoid double free.
}
public void Dispose()
{
Release();
GC.SuppressFinalize(this);
}
~Function()
{
Release();
}
}
And then MyClass would be:
public sealed class MyClass : IDisposable
{
private Library _lib;
private Function _func;
public MyClass(string dllPath)
{
_lib = new Library(dllPath); // If this fails, we throw here, and we don't need clean-up.
try
{
_func = new Function(_lib, "MyFunction");
try
{
SomeMethodThatCanThrowJustToComplicateThings();
}
catch
{
_func.Dispose();
throw;
}
}
catch
{
_lib.Dispose();
throw;
}
}
public void Dispose()
{
_func.Dispose();
_lib.Dispose();
}
}
This makes the constructor a bit more verbose, and I'd rather avoid two things that can go wrong affecting two things that need clean-up in the first place. It does though reflect why I like the clean-up to be explicit as to the different fields; I might want to clean up both fields, or just one, depending on where the exception hits.
Collected from the Internet
Please contact [email protected] to delete if infringement.
Comments