Decorators and IDisposable

Question

I have a subclass of DbContext

public class MyContext : DbContext { }

and I have an IUnitOfWork abstraction around MyContext that implements IDisposable to ensure that references such as MyContext are disposed of at the appropriate time

public interface IUnitOfWork : IDisposable { }

public class UnitOfWork : IUnitOfWork 
{
    private readonly MyContext _context;

    public UnitOfWork()
    {
        _context = new MyContext();
    }

    ~UnitOfWork()
    {
        Dispose(false);
    }

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

    private bool _disposed;

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed) return;

        if (disposing)
        {
            if (_context != null) _context.Dispose();
        }

        _disposed = true;
    }
}

My UnitOfWork is registered with a lifetime scope of per (web) request. I have decorators of IUnitOfWork that could be registered as transient or lifetime scoped and my question is what should they do with regard to implementing IDisposable - specifically should they or should they not pass on the call to Dispose().

public class UnitOfWorkDecorator : IUnitOfWork
{
    private readonly IUnitOfWork _decorated;

    public UnitOfWorkDecorator(IUnitOfWork decorated)
    {
        _decorated = decorated;
    }

    public void Dispose()
    {
        //do we pass on the call?
        _decorated.Dispose();
    }
}    

I see 2 options (I'm guessing option 2 is the correct answer):

1. It is expected that each Decorator will know whether it is transient or lifetime scoped. If a decorator is transient then it should not call Dispose() on the decorated instance. If it is lifetime scoped it should.
2. Each decorator should only be concerned with disposing of itself and should never pass on the call to the decorated instance. The container will manage the call to Dispose() for each object in the call chain at the appropriate time. An object should only Dispose() of instances that it encapsulates and decorating is not encapsulation.

Answer 1

Not an answer, but your UnitOfWork can be simplified a lot.

• Since the class itself doesn't have any native resources, there's no need for it have a finalizer. The finalizer can therefore be removed.
• The contract of the IDisposable interface states that it is valid for Dispose to be called multiple times. This should not result in an exception or any other observable behavior. You can therefore remove the _disposed flag and the if (_disposed) check.
• The _context field will always be initialized when the constructor succeeds succesfully and Dispose can never be called when the constructor throws an exception. The if (_context != null) check is therefore redundant. Since DbContext can safely be disposed multiple times, there's no need to nullify it.
• Implementing the Dispose Pattern (with the protected Dispose(bool) method) is only needed when the type is intended to be inherited. The pattern is especially useful for types that are part of a reusable framework, since there's no control over who inherits from that type. If you make this type sealed, you can safely remove the protected Dispose(bool) method and move its logic into the public Dispose() method.
• Since the type does not contain a finalizer and can't be inherited, you can remove the call to GC.SuppressFinalize.

When following these steps, this is what's left of the UnitOfWork type:

public sealed class UnitOfWork : IUnitOfWork, IDisposable
{
    private readonly MyContext _context;

    public UnitOfWork()
    {
        _context = new MyContext();
    }

    public void Dispose()
    {
        _context.Dispose();
    }
}

In case you move the creation of MyContext out of UnitOfWork by injecting it into UnitOfWork, you can even simplify UnitOfWork to the following:

public sealed class UnitOfWork : IUnitOfWork 
{
    private readonly MyContext _context;

    public UnitOfWork(MyContext context)
    {
        _context = context;
    }
}

Since UnitOfWork accepts a MyContext it doesn't have the ownership over, it is not allowed to dispose MyContext (since another consumer might still require its use, even after UnitOfWork goes out of scope). This means that UnitOfWork doesn't need to dispose anything and therefore doesn't need to implement IDisposable.

This of course means that we move the responsibility of disposing the MyContext up to 'someone else'. This 'someone' will typically be the same one that was in control over the creation and disposal of UnitOfWork as well. Typically this is the Composition Root.

Answer 2

Personally, I suspect you need to handle this on a case-by-case basis. Some decorators might have good reasons to understand scoping; for most, it is probably a good default to simply pass it along. Very few should explicitly never dispose the chain - the main times I've seen that it was specifically to counteract a scenario where another decorator that should have considered scoping: didn't (always disposed).

As a related example - consider things like GZipStream - for most people, they are only dealing with one logical chunk - so defaulting to "dispose the stream" is fine; but this decision is available via a constructor overload which lets you tell it how to behave. In recent versions of C# with optional parameters, this could be done in a single constructor.

Option 2 is problematic, as it requires you (or the container) to keep track of all the intermediate objects; if your container does that conveniently, then fine - but also note that they must be disposed in the correct order (outer to inner). Because in a decorator chain, there may be pending operations - scheduled to be flushed downstream on request, or (as a last resort) during dispose.