Is it a leaky abstraction if implementation of interface calls Dispose

Question

Consider this code:

public class MyClass()
{
  public MyClass()
  {    
  }

  public DoSomething()
  {
    using (var service = new CustomerCreditServiceClient())
    {
       var creditLimit = service.GetCreditLimit(
         customer.Firstname, customer.Surname, customer.DateOfBirth);       
    }
  }
}

We now want to refactor it to loosely couple it. We end up with this:

public class MyClass()
{
  private readonly ICustomerCreditService service;

  public MyClass(ICustomerCreditService service)
  {
     this.service= service;
  }

  public DoSomething()
  {
     var creditLimit = service.GetCreditLimit(
       customer.Firstname, customer.Surname, customer.DateOfBirth);       
  }
}

Looks ok right? Now any implementation can use the interface and all is good.

What if I now say that the implementation is a WCF class and that the using statement before the refactoring was done was there for a reason. ie/to close the WCF connection.

So now our interface has to implement a Dispose method call or we use a factory interface to get the implementation and put a using statement around that.

To me (although new to the subject) this seems like a leaky abstraction. We are having to put method calls in our code just for the sake of the way the implementation is handling stuff.

Could someone help me understand this and confirm whether I'm right or wrong.

Thanks

Millepore

You can make your interface inherit from Dispose – Tamelatameless

True or implement its own Dispose. Either way, having to call Dispose is leaky isnt it? – Millepore

Answer 1

Yes, it is a leaky abstraction when you let ICustomerCreditService implement IDisposable, since you've now written ICustomerCreditService with a specific implementation in mind. Further more, this communicates to the consumer of that interface that it could dispose that service, which might not be correct, especially since in general, a resource should be disposed by the one who creates it (who has the ownership). When you inject a resource into a class (using constructor injection for instance), it is not clear if the consumer is given the ownership.

So in general the one responsible of creating that resource should dispose it.

However, in your case, you can simply prevent this from even happening by implementing a non-disposable implementation of ICustomerCreditServiceClient that simply creates and disposes the WCF client within the same method call. This makes everything much easier:

public class WcfCustomerCreditServiceClient
    : ICustomerCreditServiceClient
{
    public CreditLimit GetCreditLimit(Customer customer)
    {
        using (var service = new CustomerCreditServiceClient())
        {
            return service.GetCreditLimit(customer.Firstname,
                customer.Surname, customer.DateOfBirth);       
        }
    }
}

Lamarlamarck

Interesting idea. The only thing I would say there is that there is another level of abstraction but that might not be a bad thing per se – Millepore

@Jon: There is no new level of abstraction. You already defined that ICustomerCreditServiceClient abstraction. The difference is that your WCF proxy doesn't implement it, but no new abstractions where added. Doing things like this prevents you from complicating you IoC configuration. – Lamarlamarck

Maybe my terminiology is wrong. I meant another level, so MyClass calls WcfCustomerCreditServiceClient which calls CustomerCreditServiceClient – Millepore

@Jon: True. But will this ever be a problem? – Lamarlamarck

If this is the only function called on that interface this was also my first idea. However, when there is not only one call to the interface but several (withing then original using-block), then this might become slow, as every call connects and disconnects... – Correspondent

@FrankB: That's true. When the performance gets a problem, you will need to move the scope up of the resource. One step up is to use a factory (as you wrote in your answer). Next step up is to let the DI/IoC container dispose it. For instance, when running in a web environment, you can register it as a 'per web request' lifestyle, which means only one instance is created during the lifetime of a single web request. If you're not running in a web environment, you'd probably need some scoping, as Dennis Traub explains. – Lamarlamarck

While factories are in general a good solution (and are easy to follow), they force you to have another abstraction in your system. So in this case I would normally start with the wrapper implementation in my answer and if that isn't fast enough, move to a 'per web request' or 'lifetime scope' lifestyle, as Dennis explains. – Lamarlamarck

Answer 2

You should handle the lifecycle of the customerCreditService in the calling code. How should MyClass know if the service is still needed by the caller? If caller is responsible for cleaning up its resources then MyClass doesn't need to be disposable.

// calling method

using (var service = new CustomerCreditServiceClient()) {
    var myClass = new MyClass(service);
    myClass.DoSomething();
}

Update: In the comments OP mentioned the use of an IoC like Ninject. The code then could look like this:

IKernel kernel = ...;

using (var block = kernel.BeginBlock())
{
    var service = block.Get<ICustomerCreditService>();
    var myClass = new MyClass(service);
    myClass.DoSomething();
}

The kernel.BeginBlock() creates an activation block. It makes sure that the resolved instances are disposed when the block ends.

Eileneeilis

But this will all be done with an IOC container so you'd need to specify something with that. I think Ninject allows you to call a dispose on a dependency implementation when you configure it – Millepore

Then you can have the IoC container resolve and dispose of the CustomerCreditServiceClient. There still is no need to implement IDisposable.Dispose() in MyClass. – Eileneeilis

You would not pass the the kernel around like that – Millepore

This is just a simplified example. You never mentioned an IoC container in the first place and I just tried to extend my answer according to your comment. Please update your question if the use of a container is so important. OTOH: Using an IoC container is considered an anti-pattern itself. – Eileneeilis

Answer 3

You should call the Dispose of the ICustomerCreditService where it has been instantiated as MyClass now has no idea of the lifecycle of ICustomerCreditService.

Diplomacy

But this will all be done with an IOC container so you'd need to specify something with that. I think Ninject allows you to call a dispose on a dependency implementation when you configure it – Millepore

Answer 4

Starting again with the first implementation, I would try to add a getInterface-Request to the Class, so that the implementation could stay more or less the same. Then it can safely call Dispose (effectively it only defers the creation of the interface implementation, but stays in control of its life cycle): (c#-code not verified...)

public class MyClass()
{
  public delegate ICustomerCreditService InterfaceGetter;
  private InterfceGetter getInterface;
  public MyClass(InterfaceGetter iget)
  {
    getInterface = iget;
  }
  public DoSomething()
  {
    using (var customerCreditService = getInterface())
    {
       var creditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);       
    }
  }
}

Correspondent

I find your answer a bit vague, but you're basically advising to inject a factory that allows creating that instance (+1 for that). In general, factories are good, since it is clear that the caller gets the ownership of the instance created, and it is clear it needs to dispose that instance. You'll still need to implement IDisposable on the ICustomerCreditService though, which is not needed in this case. – Lamarlamarck

Answer 5

Yes, it is. But that’s a necessary evil. The very existence of the IDisposable interface is a leaky abstraction. Leaky abstractions are simply an everyday fact of programming. Avoid them where possible but don’t fret when you can’t – they are ubiquitous anyway.

Gunwale

@Correspondent Uhm … could you elaborate? After all, this isn’t an opinion with which to agree or disagree, it’s a fact – facts can’t be disagreed with, only their evaluation. If you think I’ve made a mistake in the facts or their evaluation then please tell me so and I’ll correct that. – Gunwale

@Lamarlamarck Same for you. See previous comment. Incidentally, I’ve seen your answer just now, and this is indeed a non-leaky way to handle this particular case. My answer was more general though. – Gunwale

@KonradRudolph IDisposable is there exactly to be non-leaky. If you mean that it is easy to do it wrong; then yes, that's bad. But thats not IDisposables fault... It is a clean way of keeping unmanaged stuff under control. btw: You did not elaborate on what makes it leaky? – Correspondent

@Correspondent That’s not what “leaky abstraction” means. It has got nothing to do with resource leaks. Leaky abstraction means leaking implementation details (such as: “this resource needs to be disposed manually”) to the public interface. – Gunwale

@KonradRudolph ok, understood. However, I still cant completely agree to your overall statement... (but that's off topic for this question) – Correspondent

IDisposable is by itself not a leaky abstraction. IDisposable is a clear contract that communicates that the type implementing it has resources that can be deterministically cleaned up. However, when you apply IDisposable to another interface, that interface can become a leaky abstraction. The reason is that there's no way to foresee all possible implementations of an interface and therefore you can always come up with a disposable implementation of practically any interface. – Lamarlamarck

When you implement IDisposable on an interface you are leaking knowlegde of an implementation through to the interface design. However, when you implement IDisposable on an implementation, you don't have a leaky abstraction. You just state that this implementation manages resources. Therefore, IDisposable itself is not a leaky abstraction, and that's why I don't agree with you. – Lamarlamarck

@Steve Thanks for clearing this up. But this leaves us in an interesting situation since I claim that yes, IDisposable is always a leaky abstraction, at its most fundamental level. It signals something about an object for which the user of the object doesn’t care about the least bit; namely, that it requires explicit resource management. This isn’t the purpose of an object, it’s an inconvenient detail. – Gunwale

@KonradRudolph: The user of the abstraction doesn't care about IDisposable (that's why an interface should not implement IDisposable), but the user of the implementation does care about IDisposable, since he is responsible of creating and disposing it. A leaky abstraction means that implementation details are leaking through the abstraction. However, when you implement IDisposable on the implementation, there is no knowledge leaking; just communicating what is needed. – Lamarlamarck

@Steve If I’m using System.Drawing.Graphics then I’m the user of that class. Yet that class implements (needs to implement) IDisposable. Thus Graphics leaks details about its internal implementation (in terms of GDI+ by the operating system, where handles have to be allocated and disposed). – Gunwale

@KonradRudolph: The fact that some objects need deterministic disposal is an intrinsic part of how .NET (or perhaps even any computer system) works. If you take this abstraction (over the hardware) into consideration, than yes; that is indeed a leaky abstraction. In that case resource management itself is the abstraction, while I was talking about the ICustomerCreditService as abstraction. Conclusion: we were both right :-) – Lamarlamarck

@KonradRudolph You almost have me - but to turn this on its head: You can have an implementation where the Dispose does nothing (because that implementation does not need it). In this case the user thinks he knows something about the implementation, while in fact doesnt. IDisposable just asks the user to tell it when finished. Isn't that more of a behaviour (I didnt find a better term) than a leak? – Correspondent

@Steve Yes, resource management is a leaky abstraction. Joel Spolsky has written a seminal essay about this where he concludes “All non-trivial abstractions, to some degree, are leaky.” – and he’s absolutely right. – Gunwale