What's the best way of returning constructed IDisposables safely?

Question

Edit: Two options shown below.

If you're just using the functionality that an IDisposable provides, the aptly named using clause works fine. If you're wrapping an IDisposable in an object, the containing object itself needs to be IDisposable and you need to implement the appropriate pattern (either a sealed IDisposable class, or the messier but standard virtual pattern).

But sometimes a helper factory method is good for cleanliness. If you return an IDisposable directly after construction, you're fine, but if you first construct it and then modify it or otherwise execute code that can throw an exception before returning, you need to safely call .Dispose() - but only if there was an error.

For example, unsafe code could look like this...

DbCommand CreateCommandUnsafely(string commandText)
{
    var newCommand = connection.CreateCommand();
    newCommand.CommandText = commandText;  //what if this throws?
    return newCommand;
}

Solutions Two safe variants follows...

DbCommand CreateCommandSafelyA(string commandText)
{
    DbCommand newCommand = null;
    bool success = false;
    try    {
        newCommand = connection.CreateCommand();
        newCommand.CommandText = commandText; //if this throws...
        success=true;
        return newCommand;
    } finally{
        if (!success && newCommand != null )
            newCommand.Dispose(); //...we'll clean up here.
    }
}


DbCommand CreateCommandSafelyB(string commandText)
{
    DbCommand newCommand = null;
    try    {
        newCommand = connection.CreateCommand();
        newCommand.CommandText = commandText; //if this throws...
        return newCommand;
    } catch {
        if (newCommand != null)
            newCommand.Dispose(); //...we'll clean up here.
        throw;
    }
}

Safe variant A is just one line longer, but seems to be the idiomatic approach. There don't seem to be any really concise solutions, although some posters below give some lambda-using options that extract the encapsulate this logic.

The code bloat with any of the above safe methods remains, and is particularly aggravating with code that originally looked like...

return new MyDisposableThing {
    OptionA = "X",
    OptionB = B.Blabla,
    Values = src.Values.Where(priority => priority > 1.0),
};

The above code gets written safely is quite a bit longer and less readable because you can no longer safely use the shortened setter syntax.

Answer 1

No - I think there isn't a better way.

However, you could write a helper class:

public static class DisposeHelper
{
  public static TDisposable DisposeOnError<TDisposable>(TDisposable dispoable, Action<TDisposable> action)
     where TDisposable : IDisposable
  {
    try
    {
       action(dispoable);
    }
    catch(Exception)
    {
       disposable.Dispose();
       throw;
    }

    return disposable;
  }
}

So you could write:

return DisposeHelper.DisposeOnError(connection.CreateCommand(), cmd => cmd.CommandText = commandText);

I am not sure, however, if that is really a better way.

Answer 2

I believe this is the standard pattern:

DbCommand CreateCommand(string commandText)
{
    DbCommand newCommand = null;
    bool success = false;
    try
    {
        newCommand = connection.CreateCommand();
        newCommand.CommandText = commandText;
        success = true;
        return newCommand;
    }
    finally
    {
        if (!success & newCommand != null)
            newCommand.Dispose();
     }
}

It does not catch and rethrow the error.

Answer 3

You could consider writing an Extension Method:

public static class Disposable
{
    public static void SafelyDo<T>(this T disp, Action<T> action) where T : IDisposable
    {
        try
        {
            action(disp);
        }
        catch
        {
            disp.Dispose();
            throw;
        }
    }
}

This would allow you to write code like this:

var disp = new MyDisposable();
disp.SafelyDo(d =>
    {
        d.Foo = "Ploeh";
        d.Bar = 42;
    });
return disp;

Answer 4

I think you are overcomplicating the issue.

If your method returns a disposable object, then you are saying "I hereby relinquish ownership of this object, for better or worse". If an error occurs while you are building it, then why should that make a difference? The calling code will still dispose of it even if you throw an exception.

For example:

DbCommand CreateCommand(string commandText) {
    var newCommand = connection.CreateCommand();
    newCommand.CommandText = commandText; // what if this throws?
    return newCommand;
}

void UseCommand() {
    using(var cmd = CreateCommand("my query goes here")) {
        // consume the command
    }
}

Edit: Unfortunately, if an exception is thrown inside CreateCommand, the cmd variable is never set and the object will not be disposed correctly.