ensuring a well-defined object state when a constructor throws an exception

Question

I have a Visual Studio 2008 C# .NET 2.0CF application. I'm using a component base from which two concrete components are derived. The application first attempts to use SomeDisposableComponent. Its constructor throws an exception because it requires a feature that isn't available. Then, the application tries SomeOtherDisposableComponent. Its construction succeeds.

The problem is that the first component's constructor already added itself to the form's container of components before the exception was thrown. So, when the form is disposed the first component's Dispose() member is called even though the object was never fully constructed. That causes problems for the second component's destructor.

How can I ensure that when the first component throws an exception on construction, the references to it are removed?

public abstract class SomeDisposableComponentBase : Component
{
    private System.ComponentModel.IContainer components;

    private SomeInternalDisposable s_ = new SomeInternalDisposable();

    protected SomeDisposableComponentBase()
    {
        Initializecomponent();
    }

    protected SomeDisposableComponentBase(IContainer container)
    {
        container.Add(this);
        Initializecomponent();
    }

    private void InitializeComponent()
    {
        components = new System.ComponentModel.Container();
    }

    protected abstract void Foo();

    #region IDisposable Members
    bool disposed_;

    protected override void Dispose(bool disposing)
    {
        // called twice. the first time for the component that failed to initialize properly.
        // the second for the one that was used.
        if (!disposed_)
        {
            if (disposing && (components != null))
            {
                components.Dispose();
            }

            // on the second call, this throws an exception because it is already disposed.
            s_.Close();
            disposed_ = true;
        }
        base.Dispose(disposing);
    }
    #endregion    
}

public SomeDisposableComponent : SomeDisposableComponentBase
{
    public SomeDisposableComponent() : base()
    {
    }

    public SomeDisposableComponent(IContainer container) : base(container)
    {
        // This will throw an exception if it requires a feature that isn't available.
        SomeInitFunction();
    }

    protected override void Foo()
    {
        // Do something...
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);
    }
}

public partial class my_form : Form
{
    private SomeDisposableComponentBase d_;

    public my_form()
    {
        InitializeComponent();
        if (null == components)
            components = new System.ComponentModel.Container();

        try
        {
            // try the default component
            d_ = new SomeDisposableComponent(components);
        }
        catch (System.Exception)
        {
            try
            {
                // the default component requires some feature that isn't available. Try a
                // backup component.
                d_ = new SomeOtherDisposableComponent(components);
            }
            catch (System.Exception e)
            {
                // display error to the user if no suitable component can be found.
            }
        }
    }

    /// exit button clicked
    private void Exit_Click(object sender, EventArgs e)
    {
        this.Close();
    }

    /// from the my_form.designer.cs
    protected override void Dispose(bool disposing)
    {
        if (disposing && (components != null))
        {
            // this function is executed as expected when the form is closed
            components.Dispose();
        }
        base.Dispose(disposing);
    }
}

Thanks, PaulH

---

Edit: Removed unused code

The container within the SomeDisposableComponentBase was confusing. It's not relevant to the issue and I should have removed it earlier.

public abstract class SomeDisposableComponentBase : Component
{       
    private SomeInternalDisposable s_ = new SomeInternalDisposable();

    protected SomeDisposableComponentBase()
    {
    }

    protected SomeDisposableComponentBase(IContainer container)
    {
        container.Add(this);
    }

    protected abstract void Foo();

    #region IDisposable Members
    bool disposed_;

    protected override void Dispose(bool disposing)
    {
        // called twice. the first time for the component that failed to initialize properly.
        // the second for the one that was used.
        if (!disposed_)
        {
            if (disposing)
            {
                // on the second call, this throws an exception because it is already disposed.
                s_.Close();
            }
            disposed_ = true;
        }
        base.Dispose(disposing);
    }
    #endregion    
}

public SomeDisposableComponent : SomeDisposableComponentBase
{
    public SomeDisposableComponent() : base()
    {
    }

    public SomeDisposableComponent(IContainer container) : base(container)
    {
        // This will throw an exception if it requires a feature that isn't available.
        SomeInitFunction();
    }

    protected override void Foo()
    {
        // Do something...
    }

    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);
    }
}

public partial class my_form : Form
{
    private SomeDisposableComponentBase d_;

    public my_form()
    {
        InitializeComponent();
        if (null == components)
            components = new System.ComponentModel.Container();

        try
        {
            // try the default component
            d_ = new SomeDisposableComponent(components);
        }
        catch (System.Exception)
        {
            try
            {
                // the default component requires some feature that isn't available. Try a
                // backup component.
                d_ = new SomeOtherDisposableComponent(components);
            }
            catch (System.Exception e)
            {
                // display error to the user if no suitable component can be found.
            }
        }
    }

    /// exit button clicked
    private void Exit_Click(object sender, EventArgs e)
    {
        this.Close();
    }

    /// from the my_form.designer.cs
    protected override void Dispose(bool disposing)
    {
        if (disposing && (components != null))
        {
            // this function is executed as expected when the form is closed
            components.Dispose();
        }
        base.Dispose(disposing);
    }
}

Answer 1

Why does container.Add(this) appear before InitializeComponent()? By definition, you're adding an uninitialized component to the container; that's likely to cause problems even if the initialize method doesn't fail.

Just switch the order:

    InitializeComponent();
    container.Add(this);

That way, if InitializeComponent throws, the component will never add itself to the container. No need for special cleanup.

I'm not sure why you need that constructor at all, but if for some reason it has to be in the order that you already have (i.e. the InitializeComponent method depends on the container... eek), then put the exception handling in the constructor itself:

try
{
    container.Add(this);
    InitializeComponent();
}
catch (WhateverException)
{
    container.Remove(this);
}

But don't do this unless you really have to; much better to just not add the component to the container until it's fully initialized and stable.

Answer 2

Your form should be in charge of adding items to its components collection - not the components you're adding to the collection. Doing it inside the added components makes it extremely difficult to reason about what's happening in your form.

// It is not obvious that the component is adding itself to the list
new SomeDisposableComponent(components);

// Instead, be explicit where it matters
Component someDisposableComponent = new SomeDisposableComponent(components);
this.components.Add(someDisposableComponent);

Next, to solve your constructor problem, you should move the logic out of the constructor. This is generally good practice - people do not expect constructors to have side effects, and logic in your constructors makes classes difficult to test. If you want to guarantee that something happens every time you create an instance, create a factory and make it so the factory is the only way to get an instance of your class (make the class' constructor internal, or use some other technique):

public class SomeDisposableComponentFactory {
    public SomeDisposableComponent CreateInstance() {
        SomeDisposableComponent component = new SomeDisposableComponent();
        component.SomeInitFunction();
    }
}

Finally, you should consider moving the logic for selecting and creating components out of your form and into a general component factory:

public class ComponentFactory {

    // The input parameter can be whatever you need to choose the right component
    public Component CreateInstance(object input) {

        if (input == something) {
            SomeDisposableComponent component = new SomeDisposableComponent();
            component.SomeInitFunction();
            return component;
        }
        else {
            return new AnotherComponent();
        }
    }
}