Calling Dispose method inside constructor which throws an exception or behaves unexpectedly

Question

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");
         }
     } 

     // ...
} 

Answer 1

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.

Answer 2

What you describe is the pattern implemented by the C++/CLI compiler, which should have been standard in all .NET languages but is not. The failure of .NET to specify that a failed constructor should result in a call to Dispose on the partially-constructed object (and that any legitimate Dispose implementation must be prepared to deal with this) means that many kinds of objects must either require the use of factory methods rather than constructors, require an awkward two-step construction sequence in which objects are in a weird "limbo" until the second step completes, or adopt a "hope nothing goes too badly wrong wrong" philosophy for error handling and cleanup.

Of these approaches, the best is probably to require the use of a factory method for construction. Because factory methods are specific to the type of object being created, this approach require derived classes to include some annoying boilerplate to the effect of:

DerivedFoo Create(params)
{
  // Phase 1 shouldn't allocate resources yet
  Derived foo result = new DerivedFoo(params);
  // NON-VIRTUAL Phase 2 method which chains to a virtual one within try/finally
  result.Initialize();
  return result;
}

Not totally horrible, but irksome. The .NET Framework could have benefited enormously from allowing classes to specify an Initialize method which would be invoked between the completion of the most-derived constructor and the return to client code, but since no such feature "officially" exists the best one can do is probably to kludge it manually [I think there are some kludges designed for COM inter-op that might help, but I don't know how well supported there are].