Decorators and IDisposable
what should [decorators] do with regard to implementing IDisposable
This comes back to the general principle of ownership. Ask yourself: "who owns that disposable type?". The answer to this question is: He who owns the type is responsible for disposing of it.
Since a disposable type is passed on to the decorator from the outside, the decorator didn't create that type and should normally not be responsible for cleaning it up. The decorator has no way of knowing whether the type should be disposed of (since it doesn't control its lifetime) and this is very clear in your case, since the decorator can be registered as transient, while the decoratee has a much longer lifetime. In your case your system will simply break if you dispose the decoratee from within the decorator.
So the decorator should never dispose the decoratee, simply because it doesn't own the decoratee. It's the responsibility of your Composition Root to dispose that decoratee. It doesn't matter that we're talking about decorators in this case; it still comes down to the general principle of ownership.
Each decorator should only be concerned with disposing of itself and should never pass on the call to the decorated instance.
Correct. The decorator should dispose everything it owns though, but since you're using dependency injection, it typically doesn't create much stuff itself and therefore doesn't own that stuff.
Your UnitOfWork
on the other hand creates a new MyContext
class and therefor has the ownership of that instance and it should dispose of it.
There are exceptions to this rule, but it still comes down to ownership. Sometimes you do pass on ownership of a type to others. When using a factory method for instance, by convention the factory method passes on the ownership of the created object to the caller. Sometimes ownership is passed on to a created object, such as .NET's StreamReader
class does. The API documentation is clear about this, but since the design is such unintuitive, developers keep tripping over this behavior. Most of the types in the .NET framework don't work this way. For instance, the SqlCommand
class doesn't dispose the SqlConnection
, and it would be very annoying if it did dispose of the connection.
A different way of looking at this issue is from perspective of the SOLID principles. By letting the IUnitOfWork
implement IDisposable
you are violating the Dependency Inversion Principle, because "Abstractions should not depend on details; Details should depend on abstractions". By implementing IDisposable
you are leaking implementation details into the IUnitOfWork
interface. Implementing IDisposable
means that the class has unmanaged resources that need disposal, such as file handles and connection strings. These are implementation details, because it can't hardly ever be the case that each implementation of such interface actually needs disposal at all. You just have to create one fake or mock implementation for your unit tests and you have proof of an implementation that doesn't need disposal.
So when you fix this DIP violation by removing the IDisposable
interface from IUnitOfWork
-and moving it to the implementation-, it becomes impossible for the decorator to dispose the decoratee, because it has no way of knowing whether or not the decoratee implements IDisposable
. And this is good, because according to the DIP, the decorator shouldn't know -and- we already established that the decorator should not dispose the decoratee.
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 forDispose
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 theif (_disposed)
check. - The
_context
field will always be initialized when the constructor succeeds succesfully andDispose
can never be called when the constructor throws an exception. Theif (_context != null)
check is therefore redundant. SinceDbContext
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 typesealed
, you can safely remove the protectedDispose(bool)
method and move its logic into the publicDispose()
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.