4

What is the best way to properly deal with a default implementation of a interface when the interface doesn't inherit from IDisposable? For example, suppose I want to do

public class FooGetter : IDisposable {

    private IFooProvider fooProvider = MyContainer.GetDefault<IFooProvider>();
    ...
    public void Dispose(){
         ...
         if (fooProvider != null) fooProvider.Dispose(); // obviously has compile error here
    }
}

And it just so happens that the default implementation of IFooProvider is IDisposable, but IFooProvider interface does not inherit from IDisposable. How/where am I supposed to dispose of it?

The question isn't just for dependency injection containers; it would also apply to a tightly-coupled dependency:

private IFooProvider fooProvider = new PatrickProvider();

In this case, I could keep another reference so that I can later Dispose() it, but that seems really janky:

private PatrickProvider defaultFooProvider = new PatrickProvider();
private IFooProvider fooProvider = defaultFooProvider;

Looking for best (or good) practices here.

Patrick Szalapski
  • 8,738
  • 11
  • 67
  • 129
  • Related: http://stackoverflow.com/questions/987761/how-do-you-reconcile-idisposable-and-ioc – Patrick Szalapski May 04 '11 at 21:54
  • Are you in control of IFooProvider? Of PatrickProvider? Are you looking for best practices in designing them or only in using them as they stand? – David Ruttka May 05 '11 at 00:00
  • Using them as they stand--suppose that IFooProvider does not implement IDisposable, but PatrickProvider does. – Patrick Szalapski May 05 '11 at 19:53
  • Reading the linked articles -- what I gather from [link](http://objectmix.com/dotnet/97439-interface-inheritance-idisposable.html): If an interface does not inherit from IDisposable, no implementations will ever need to be disposable. Can that be right? It seems contrary to good OO thinking--lack of a property or inheritance should imply nothing. (e.g. IFooProvder lacks a Id property, yet PatrickProvider is certainly allowed to have a Id property) – Patrick Szalapski May 05 '11 at 20:29
  • 1
    That's not true at all. The `new`-er of the concrete implementation should take care of disposing, since that `new`-er (e.g. a DI container) knows how to map `IFooProvider` to a concrete implementation and furthermore has all the code specific to that particular implementation. – Domenic May 05 '11 at 21:00
  • @Domenic, thanks. So in the tightly-coupled scenario, I should go with the last code sample above--then I can Dispose of it properly. In the loosely-coupled scenario (the first code sample), MyContainer needs to handle the disposing. – Patrick Szalapski May 05 '11 at 21:27
  • 1
    Those are my thoughts, yeah :). Sometimes you also need the ability to notify the container that you are done with the object; most DI containers provide this (usually under the name `Release`). But it's still up to the container to decide whether it's time to _dispose_ or not, e.g. if an object has singleton lifecycle then it will wait for all consumers to release before disposing. – Domenic May 06 '11 at 00:40

4 Answers4

2

Best practices when using a DI container is to let your DI container handle the lifetime of the object for you. This is not possible if you use your DI container as a service locator (boo! anti-pattern!!), like you seem to be doing with MyContainer.GetDefault, but if you properly have your dependencies injected through constructor injection, then it would work well.

For tightly-coupled dependencies, you would have to do something stupid like

using(fooProvider as IDisposable)
{
    // Code that uses fooProvider
}

I think (but am not 100% sure) that using will do nothing if passed null, so this will work whether or not fooProvider implements IDisposable.

There is a good discussion of IDisposable as it relates to dependencies an DI in Mark Seemann's Dependency Injection in .NET chapter 8.

Domenic
  • 110,262
  • 41
  • 219
  • 271
1

You could do a runtime check that the object implements IDisposable and if so, dispose of it:

public class FooGetter : IDisposable
{
    private IFooProvider fooProvider = MyContainer.GetDefault<IFooProvider>();
    ...
    public void Dispose()
    {
         IDisposable disposable = fooProvider as IDisposable;

         if (disposable != null)
         {
             fooProvider.Dispose();
         }
    }
}

You still need to have an inkling that the implementation of your interface may be disposable otherwise the code is a bit pointless, but for your case it will ensure that Dispose() is called on implementations that have it.

Ideally, your interface should derive from IDisposable to state that part of its contract is that you must dispose of it when finished, but I acknowledge that in reality this may not always be possible.

adrianbanks
  • 81,306
  • 22
  • 176
  • 206
  • "Ideally, your interface should derive from IDisposable" -- that's what I want to avoid, since a class need not inherit from IDisposable in order to provide Foo. druttka has my thought in mind. – Patrick Szalapski May 05 '11 at 20:02
  • @Patrick: As I said, "...I acknowledge that in reality this may not always be possible". – adrianbanks May 05 '11 at 20:05
1

To me it seems that there are a few things to consider.

First, it is perfectly reasonable to imagine an interface IFoo where some implementations might have resources to dispose of or cleanup to perform while others might not; therefore, it is some consider it reasonable not to make IFoo inherit IDisposable.

public interface IFoo{
    // ...
}

// Has no cleanup or resources
public interface Foo1:IFoo{
    // ...
}

// Does have resources to release and cleanup to perform
public interface Foo2:IFoo,IDisposable{
    // ...
}

See this answer and this discussion for views of whether the choice to implement IDisposable should be up to concrete classes or up to abstract classes / interfaces. EDIT: Struck most of this paragraph because after reviewing the discussions, I see good points on the "have the interface define it" side and will be reevaluating my opinion. Plus, that decision isn't really relevant to the question being asked.

Now all that aside, if you have IFoo x, and some implementations implement IDisposable while others do not, you can check whether x implements IDisposable. In the case that it does, properly dispose of it. If not, then you should trust that whoever wrote x's implementation of IFooProvider did not implement IDisposable because you need not call it, and be secure in the knowledge that you've done all you can.

IFoo x; 
//x = whatever
if (x is IDisposable)
{
    ((IDisposable)x).Dispose();
}
Community
  • 1
  • 1
David Ruttka
  • 14,269
  • 2
  • 44
  • 39
  • I dislike the latter code. Post #4 of your second linked discussion says it best. If the return type of a factory method or constructor implements IDisposable, that creates a strong inference that the caller needs to ensure that created objects get disposed. Conversely, if the return type of a factory method or constructor doesn't implement IDisposable, *that creates a strong inference that it can safely be created and abandoned*. – supercat May 04 '11 at 22:33
  • I agree with that entirely and that's part of why I favor defining it on the abstraction more today than yesterday. In the case where you aren't in control of the existing interface and implementations, though, but happen to know that some but not all implementations also implement IDisposable, some kind of type check would be necessary? – David Ruttka May 04 '11 at 23:55
  • Also the reason I can agree with it entirely is because it says "strong inference" and not "guarantee" – David Ruttka May 04 '11 at 23:56
  • @druttka: I'm reminded of a sign at a railroad crossing: "When bell is ringing stop, look, and listen. When bell is not ringing stop, look, and listen, in case the bell has failed." What exactly is the purpose of the bell? – supercat May 05 '11 at 03:29
  • I'm deleting my most recent three comments because after rereading them, they aren't very well written. I'll revise and respond later. In the meantime, I respect your position and do agree with you IF the OP is defining IFooProvider. That's why I upvoted your answer. I am thinking of the angle where it's not under his control. I've put a comment under the question itself asking for clarification of what situation he is in. – David Ruttka May 05 '11 at 14:03
  • 1
    @supercat The purpose of the bell, when operational, is to draw attention to the situation. The danger is relying only upon the bell and disregarding what is in plain sight. Using your example, failing to dispose of something when you should is like getting hit by the train. We agree that a method returning IDisposable is a strong inference that you should (a bell). In some cases (Patrick has one, I believe), a returned interface doesn't inherit IDisposable, but some implemenations of that interface do (the bell has failed). One needs to use one's own judgment to take the appropriate action. – David Ruttka May 05 '11 at 14:40
  • @druttka: I wonder if one should define an extension method DisposeIfDisposable, which would cast as disposable and Dispose if non-null, or define a global static method for such a purpose? It's too bad overload resolution can't operate on constraints, because I like to use a generic Zap() function with a byref argument, which 'Interlocked.Exchange's its argument to null and calls Dispose if it was non-null; one could make Zap() accept any object and attempt a typecast to IDisposable, but such a typecast would then be needed even if the object was known to be IDisposable. – supercat May 05 '11 at 15:07
  • @druttka: I'm pretty sure it's less expensive to call a do-nothing IDisposable.Dispose method on an object which is known to be IDisposable, than to attempt--successfully or not--to typecast to IDisposable an object which may or may not be. If code that calls a factory method is going to be expected to dispose the resulting object if it implements IDisposable, regardless of whether the factory is declared as returning one, it would be cheaper for the factory's return type to implement IDisposable. There are a few cases, mostly dating back to .net 1.0, where objects are maybe-disposable... – supercat May 05 '11 at 15:14
  • @druttka: ...and one must for safety use code like the above. Nonetheless, I can see very few cases where there would be any advantage whatsoever to having a factory whose declared return type doesn't implement IDisposable, but whose returned objects might need disposing. I can see no case where one wouldn't be better off having the factory's return type implement IDisposable, but also include a NeedsDisposing property (there are some cases where knowing in advance that one won't need to Dispose an object may allow some useful optimizaitons; the fact that an object implements IDisposable... – supercat May 05 '11 at 15:21
  • 1
    @druttka: ...may preclude such optimizations even if the Dispose method would do nothing). It's too bad Microsoft seems to have designed IDisposable as an afterthought, since there are some weaknesses in the design which we are by now unfortunately stuck with (e.g. if Dispose is called during the handling of an exception, and the Dispose itself fails with an exception, the best behavior would probably be for Dispose to wrap both exceptions in a CleanupException, but Dispose wouldn't have the information needed to do that). – supercat May 05 '11 at 15:27
  • Very useful discussion. Thanks, @supercat and @druttka. – Patrick Szalapski May 06 '11 at 02:18
0

If you're defining IFooProvider, why not make it inherit IDispoable? If someone comes up with an implementation which doesn't need cleanup, it can simply implement an empty Dispose method.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • Because this violates separation of concerns--IFooProvider is responsible for specifying what a class must be to provide foo. Being disposable is not always required. – Patrick Szalapski May 05 '11 at 19:54
  • @Patrick Szalapski: The IDisposable interface is in a sense backward, in that the stronger promise applies to classes which *don't* implement it. The IDisposable interface doesn't imply that a class actually needs cleanup, so much as a lack of an IDisposable interface implies that it does not. IDisposable promises to do everything necessary for cleanup; if that "everything" is nothing, it's met its obligations. If a factory method might return an object which the caller should dispose, the return type should implement IDisposable. – supercat May 05 '11 at 20:31
  • @Patrick Szalapski: If I had my druthers, rather than defining Finalize(), type Object would have a virtual "Integer Dispose(Integer)" and a static "void Dispose()" which would call Dispose(DisposeCause.Dispose), "Boolean NeedsDisposing()" which would check whether Dispose(DisposeCause.CheckNeed)" returns non-zero, etc. Other argument values would be used for object creation (allowing objects to register a finalizer), failed constructors (allowing deterministic cleanup of nested resources), etc. so there'd be no need for IDisposable. But deterministic cleanup was an afterthought in .net. – supercat May 05 '11 at 20:41
  • OK, so we should assume IDisposable on interfaces to be a backward contract--a lack of IDisposable means no disposal needed; inheriting IDisposable means nothing, so you'd better dispose it just in case. Right? – Patrick Szalapski May 05 '11 at 20:50
  • I disagree that `IDisposable` is a backward contract. I think the original meaning is correct; `IDisposable` is something you apply to certain concrete implementations that need disposing, as that is one aspect of their implementation. – Domenic May 05 '11 at 20:55
  • @Domenic You seem to be discussing concrete classes--but what about when IDisposable is on interfaces? – Patrick Szalapski May 05 '11 at 21:44
  • 1
    @Patrick Szalapski: If an interface is expected to be used as the return type of a factory method which yields an object needing disposal, then it should implement IDisposable. This most common situation this applies is when the factory itself is an interface (e.g. if IFactory is supposed to have a MakeCar function that returns an ICar, and some MakeCar functions will return a IDisposable objects, then ICar should implement IDisposable). – supercat May 05 '11 at 22:14
  • 1
    @Patrick `IDisposable` should only be applied to interfaces when you want to communicate that disposal is part of the contract of the interface. This seems exceedingly rare to me, since disposal is (almost?) always an implementation detail. The only case I could think of where it makes sense is when you are trying to do something clever with `using` as an idiom, e.g. as is done in ASP.NET MVC with `using(Html.BeginForm())`. – Domenic May 06 '11 at 00:38
  • Great, that clears it up. If anyone disagrees, please speak up. – Patrick Szalapski May 06 '11 at 02:21
  • @Domenic: Suppose I have an ICarFactory whose MakeCar method returns an ICar. Anyone who calls MakeCar will need to know what should be done with the returned object once it is no longer needed. I would aver that the normal convention is that if ICar implements IDisposable, whoever calls MakeCar is responsible for calling Dispose; if ICar does not implement IDisposable, whoever calls MakeCar may safely abandon the object. Actually, I would go further and suggest that if ICar does not implement IDisposable, that's a sign... – supercat May 06 '11 at 02:23
  • @Domenic: ...that the caller shouldn't call IDisposable.Dispose on the return from MakeCar, *even if the returned object happens to be of a type that implements IDisposable*. In some cases, factory methods may return shared instances of objects that callers aren't expected to mutate, and the bad things may happen if such a shared instance gets disposed. If ICar doesn't implement IDisposable, callers of MakeCar aren't responsible for calling IDisposable.Dispose. – supercat May 06 '11 at 02:27
  • @supercat: It would be irresponsible of the caller to dispose of something given to them by a factory. Since the caller wasn't responsible for the object's creation, it is improper for him to take on responsibility for the object's lifetime. If the factory (which is supposed to be an abstraction, recall) decides to pull `ICar` instances from a shared pool, or implement the flyweight pattern, or whatever, then a caller who irresponsibly disposes of the object given could seriously mess up any users of the factory, and breaks the factory abstraction. – Domenic May 06 '11 at 06:48
  • @supercat To clarify, we agree on most points, except I would say even if `ICar` implements `IDisposable` then users of the factory should not dispose the instances they are given. But I think that our disagreement will come up _very_ rarely in practice, because `IDisposable` shouldn't be applied to the interfaces a factory is returning. – Domenic May 06 '11 at 06:52
  • @supercat Actually I am not sure what my position is on that particular case anymore. Because my sole example where `IDiposable` is applied to an interface is the `using` idiom/trick a la `Html.BeginForm()`. And in that case the whole _point_ was to have the caller of the factory dispose of the object (via `using`). So... if there is a legitimate case where `IDiposable` is applied to an interface besides for the `using` idiom, I reserve judgement and tentatively say that my above logic holds. But since the `using` idiom is the only example we have, so far it looks like callers should dispose. – Domenic May 06 '11 at 06:56
  • @Domenic: If a factory is going to return objects that need to be disposed (a *lot* of factories do), how is anybody but the caller's factory going to know when the items are no longer needed? Most factories return concrete types, but some factory interfaces return interfaces (e.g. in a tile game, an ITileRendererFactory might return an ITileRenderer, which would support methods for drawing tiles with different highlighted states, etc.) The user of an ITileRendererFactory wouldn't get any particular benefit from requiring all tile rendering objects to inherit from a common class... – supercat May 06 '11 at 14:57
  • @Domenic: ...but some rendering objects might conceivably benefit by inheriting from some other class. Since it's likely that some tile renderers might generate and keep one or more bitmaps, the user of an ITileRenderer should let it know when it will no longer be needed; IDisposable.Dispose is the preferred way of doing that. – supercat May 06 '11 at 15:00
  • 1
    @supercat: still, I don't think `ITileRenderer` should be disposable! Disposability is an implementation detail, not part of the contract of being a tile renderer. If the user of an `ITileRenderer` needs to explicitly release an object, it should let the factory know via a much less confusing mechanism, like `TileRendererFactory.Release`. Then `TileRendererFactory` can decide whether to not dispose at all, dispose now, dispose when other consumers are done... at this point `TileRendererFactory` is a DI container of course, so you should probably be using one of those. – Domenic May 06 '11 at 15:56
  • @Domenic: I'm not clear how saying "everyone who calls ITileRendererFactory.MakeTile must ensure that Dispose gets called on the returned object", a message which would be implied by having ITileRenderer implement IDisposable, is more confusing than requiring some other protocol not involving IDisposable. The question of whether to destroy any bitmaps as soon as a user is done with a tile is indeed an implementation detail, but the caller doesn't have to worry about it. If the bitmaps are supposed to be returned to a pool rather than destroyed, the Dispose method can do that. – supercat May 06 '11 at 17:23
  • @Domenic: Perhaps I should use an example from .net itself: IEnumerable, which returns an IEnumerator, which implements IDisposable. Anyone who calls IEnumerable.GetEnumerator *must* ensure that IDisposable.Dispose gets called upon the returned value. Depending upon what the enumerable object is, very bad things (locks being held indefinitely, etc.) may happen if someone calls GetEnumerator and does not call Dispose. – supercat May 06 '11 at 17:33
  • @supercat, I think your objection gets back to the heart of my original question--that is, implementations of ITileRenderer have the single responsibility to render tiles. Now if rendering tiles inherently requires unmanaged resources or managed disposable objects, then certainly it should inherit from IDisposable. However, I'd suspect that the concept of rendering tiles is agnostic to these concerns, and therefore it shouldn't implement IDisposable. Leave that to the implementations, as @Domenic says. – Patrick Szalapski May 06 '11 at 18:03
  • @supercat The `IEnumerable`/`IEnumerator` example has me convinced! But for the record, the pool was not of bitmaps, but of `ITileRenderer` instances; the factory maybe hands out one of five shared tile renderers, and in that case having the callers call `Dispose()` on an instance they get is bad news. Combined takeaway: one should never design a factory or container that returns interfaces to which `IDisposable` is applied, if the factory's implementation results in an instance lifecycle other than caller-owned. – Domenic May 06 '11 at 18:06
  • @Domenic: The factory should only hand out shared ITileRenderer instances if they are immutable and Dispose does nothing. Otherwise, if it's going to pool things, it should hand out a wrapper that holds a shared subcomponent and does whatever is needed to ensure proper disposal. – supercat May 06 '11 at 18:42
  • @Patrick Szalapski: Almost every class object that gets created will at some point be destroyed. If class objects aren't supposed to take on the responsibility of putting their own affairs in order when they're notified that they're about to be abandoned, who's supposed to do it for them? Any other class that was going to do it would have to have intimate knowledge of exactly what needed to be done--a design far more heinous than giving classes, with few exceptions, the responsibility of putting their own affairs in order. – supercat May 06 '11 at 18:51
  • @Patrick Szalapski: Note that while most interfaces imply responsibilities for their implementers, IDisposable implies a responsibility for an object's owner. Nearly all objects have an inherent responsibility for putting their own affairs in order if they are notified that they are going to be abandoned. Implementing IDisposable is a means by which objects can let their owners know that they'll need such notification. An object which implements IDisposable has no responsibilities that it would have if it didn't implement IDisposable, but implies upon its owner responsibility to call Dispose. – supercat May 06 '11 at 18:56