Checking constraints using IDisposable -- madness or genius?

Question

I ran across a pattern in a codebase I'm working on today that initially seemed extremely clever, then later drove me insane, and now I'm wondering if there's a way to rescue the clever part while minimizing the insanity.

We have a bunch of objects that implement IContractObject, and a class InvariantChecker that looks like this:

internal class InvariantChecker : IDisposable
{
    private IContractObject obj;

    public InvariantChecker(IContractObject obj)
    {
        this.obj = obj;
    }

    public void Dispose()
    {
        if (!obj.CheckInvariants())
        {
            throw new ContractViolatedException();
        }
    }
}

internal class Foo : IContractObject
{
    private int DoWork()
    {
        using (new InvariantChecker(this))
        {
            // do some stuff
        }
        // when the Dispose() method is called here, we'll throw if the work we
        // did invalidated our state somehow
    }
}

This is used to provide a relatively painless runtime validation of state consistency. I didn't write this, but it initially seemed like a pretty cool idea.

However, the problem arises if Foo.DoWork throws an exception. When the exception is thrown, it's likely that we're in an inconsistent state, which means that the InvariantChecker also throws, hiding the original exception. This may happen several times as the exception propagates up the call stack, with an InvariantChecker at each frame hiding the exception from the frame below. In order to diagnose the problem, I had to disable the throw in the InvariantChecker, and only then could I see the original exception.

This is obviously terrible. However, is there any way to rescue the cleverness of the original idea without getting the awful exception-hiding behavior?

Answer 1

I don't like the idea of overloading the meaning of using in this way. Why not have a static method which takes a delegate type instead? So you'd write:

InvariantChecker.Check(this, () =>
{
    // do some stuff
});

Or even better, just make it an extension method:

this.CheckInvariantActions(() =>
{
    // do some stuff
});

(Note that the "this" part is needed in order to get the C# compiler to look for extension methods that are applicable to this.) This also allows you to use a "normal" method to implement the action, if you want, and use a method group conversion to create a delegate for it. You might also want to allow it to return a value if you would sometimes want to return from the body.

Now CheckInvariantActions can use something like:

action();
if (!target.CheckInvariants())
{
    throw new ContractViolatedException();
}

I would also suggest that CheckInvariants should probably throw the exception directly, rather than just returning bool - that way the exception can give information about which invariant was violated.

Answer 2

This is a horrid abuse of the using pattern. The using pattern is for disposing of unmanaged resources, not for "clever" tricks like this. I suggest just writing straight forward code.