Tuesday, March 3, 2009

Exceptions and Constructors

You should never include any work in a constructor which may leave the object in an inconsistant state. Constructors should be atomic and consistant. You should avoid having side effects in a constructor. However, sometimes you cannot avoid doing additional work in a constructor.

For example, you are designing a stream like class which acquires an unmanaged resource in the constructor. Following RAII, you implement the necessary logic to clean up your unmanaged resource in Dispose and Finalize. Given the following pseudo-code, what is a possible error condition we are not current designed to handle:
public MyType()
{
Check.Invariant(argumentInvariant1);
// ...
Check.Invariant(argumentInvariantN);

// 1. ACQUIRE
this.AcquireUnmanagedResource();

// 2. Set up our base properties from the resource
this.RetrieveConfiguration();
}

~MyType()
{
this.Dispose(false);
}

public void Dispose()
{
this.Dispose(true);
GC.SupressFinalize(this);
}

private void Dispose(bool disposing)
{
if (!this.isDisposed)
{
// RELEASE
this.ReleaseUnmanagedResource();

if (disposing)
{
this.DisposeManagedResources();
}
}
}
What would happen if #2 threw an exception? Well, it is more obvious in this example than in most code in the wild, but our Dispose or Finalize will not be called! Our unmanaged resource will not be released, and we will be leaked.

An appropriate solution would, perhaps, be the following modification to the constructor:
public MyType()
{
Check.Invariant(argumentInvariant1);
// ...
Check.Invariant(argumentInvariantN);

try
{
// 1. ACQUIRE
this.AcquireUnmanagedResource();

// 2. Set up our base properties from the resource
this.RetrieveConfiguration();
}
catch(Exception)
{
// RELEASE
this.Dispose(true);

throw; // propagate the exception
}
}
Now, if #2 should fail we will release the unmanaged resource and not leak anything. Keep in mind this will require a Dispose method which is resistant to an inconsistant object state. However, the design contract for IDisposable basically requires this anyways.

No comments: