using statement with fluent api will not call dispose if throw occurs in fluent method chain

user_1165998 0
Viewed 54

When combining a using statement with a fluent api that can potentially throw, the lowered code will never call dispose correctly.

If I have the following class that exposes a fluent interface:

public class Wrapper : IDisposable
{   
    private bool _isAdded;
    
    public Wrapper Add()
    {
        _isAdded = true;
        return this;
    }

    public void Dispose() => Console.WriteLine("dispose called");

    public Wrapper ThrowIfAdded() => _isAdded ? throw new Exception() : this;
}

and I call it with the following:

using var willNotDispose = new Wrapper().Add().ThrowIfAdded();

the lowered code will result in the Dispose call occurring after the fluent method chain is completed.

Wrapper willNotDispose = new Wrapper().Add().ThrowIfAdded();
try
{
}
finally
{
    if (willNotDispose != null)
    {
        ((IDisposable)willNotDispose).Dispose();
    }
}

Alternatively, if the call to .ThrowIfAdded() is done outside of the initial using declaration,

using var willDispose = new Wrapper().Add();
willDispose.ThrowIfAdded();

the lowered code is generated as expected.

Wrapper willDispose = new Wrapper().Add();
try
{
    willDispose.ThrowIfAdded();
}
finally
{
    if (willDispose != null)
    {
        ((IDisposable)willDispose).Dispose();
    }
}

While I understand why this is occurring, it isn't desirable. Is there any way to coerce the former initialization to compile to the latter? Ideally, it would be an attribute or form of compiler hint that would result in:

Wrapper willDispose = default;
try
{
    willDispose = new Wrapper().Add().ThrowIfAdded();
}
finally
{
    if (willDispose != null)
    {
        ((IDisposable)willDispose).Dispose();
    }
}

which I would have expected the original example to compile to in the first place.

fluent using-statement c#
1 Answers

As pointed out in the comments, there is pre-existing guidance that when an exception is thrown in a constructor, it should be explicitly handled and the resources cleaned up.

This extends to CA2000 analysis that states:

When constructors that are protected by only one exception handler are nested in the acquisition part of a using statement, a failure in the outer constructor can result in the object created by the nested constructor never being closed. In the following example, a failure in the StreamReader constructor can result in the FileStream object never being closed. CA2000 flags a violation of the rule in this case.

using (StreamReader sr = new StreamReader(new FileStream("C:/myfile.txt", FileMode.Create)))
{ ... }

While a Fluent API throwing an exception is not explicitly either a constructor or nested constructor throwing an exception, it should be treated the same since the object will be created and mutated outside of the try/finally block.

As a result, any method that can throw must first call dispose before allowing the exception to propagate.

public class Wrapper : IDisposable
{
    private bool _isDisposed;
    private bool _isAdded;

    public Wrapper Add()
    {
        _isAdded = true;
        return this;
    }

    public void Dispose()
    {
        if (_isDisposed)
        {
            return;
        }

        _isDisposed = true;
        Console.WriteLine("dispose called");
    }

    public Wrapper ThrowIfAdded()
    {
        if (_isAdded)
        {
            Dispose();
            throw new Exception();
        }

        return this;
    }
}

This correctly assures that in cases where .Added() is called, .ThrowIfAdded() will dispose prior to throwing.

If .Added() is not called, the instance will be disposed at the end of the block as expected.

user_11659980
Related