6

Many objects in .NET (SQLCommand for instance) implement IDisposable. As a general rule, if I create an instance of an IDisposable object, should I always dispose it?

Gilles 'SO- stop being evil'
  • 104,111
  • 38
  • 209
  • 254
Foo
  • 4,206
  • 10
  • 39
  • 54
  • Just ask yourself why you'd want an object that the Garbage Collector can't croak lingering around your application. – Geeky Guy Sep 12 '13 at 18:55
  • 1
    @Renan Well, there are reasons, actually, but they're the exceptions, not the rule. – Servy Sep 12 '13 at 18:56
  • 3
    Here's a good example of an exception: http://stackoverflow.com/questions/3734280/is-it-considered-acceptable-to-not-call-dispose-on-a-tpl-task-object – Matt Smith Sep 12 '13 at 19:09
  • @Matt I was looking for that one and had just found it. You beat me by a few seconds... – Dax Fohl Sep 12 '13 at 19:11

5 Answers5

17

Yes, unless you're receiving it from a caller who needs it once you're done or has taken responsibility for calling Dispose(). The important thing is that someone calls Dispose() and if you are being passed an IDisposable instance, there needs to be an understanding ("contract") about whether you are taking ownership for it (and thus need to dispose it) or whether you are "borrowing it" and the caller will use/dispose it upon your return. These are the types of things good APIs have in their documentation.

If you are instantiating the object, make it easy on yourself to automatically dispose by using using.

Omaha
  • 2,262
  • 15
  • 18
  • 1
    That would be a really badly engineered caller. – Jon Sep 12 '13 at 18:53
  • +1 for being quick and straight to the point. I suggest linking to some resource documenting the use of `using` to make your answer even better. – Geeky Guy Sep 12 '13 at 18:53
  • 6
    @Jon How so? Unless otherwise specified, it's expected that whoever created the disposable object would be responsible for disposing it. If someone passes in a disposable resource for me it's assumed they're responsible for disposing it, and if I dispose of it early then *I'm* the one that's poorly engineered. – Servy Sep 12 '13 at 18:55
  • I agree with @Servy was about to write that. – Luis Tellez Sep 12 '13 at 18:56
  • @Servy: Sure, but the caller should not be tempting you. Whatever they need to pass out should not be typed as `IDisposable` or something implementing it unless it's really meant to be disposed. They should pass you an `IWhatever` instead. Why invite bugs for no benefit? – Jon Sep 12 '13 at 18:57
  • 1
    @Jon I strongly disagree. In many cases it makes sense to pass around say a `DataContext`, or a `Task`, or an `IEnumerator`, or a `Stream`, or a `Bitmap`, or whatever. It's quite common that it's not possible (because you don't have control over the type's definition), or not practical, to have an interface that defines every single aspect of it's behavior bar disposal just so that nobody else could possibly dispose of it. – Servy Sep 12 '13 at 18:59
  • @Servy: Those things that you mention are all meant to be disposed by the recipient. And it's trivially easy to hide `IDisposable` if you want to. Instead of `class D : IDisposable` you write `class O` and `class I : O, IDisposable`. You use `I` internally and pass `O` to the outside world. – Jon Sep 12 '13 at 19:02
  • @Servy: Also, this discussion reminds me of "what the target for `lock` should be". You can easily `lock(this)`, but this invites a deadlock if some external code on another thread locks on you as well. While that would be a bug on *their* part, you don't see people refraining from the recommended "create a private object for a lock target". All the framework classes do this. – Jon Sep 12 '13 at 19:05
  • @Jon There's not a general case to receive an argument of type IDisposable (an object who's sole purpose is to be disposed), so this would make the case of a further derivation a moot point. If I were of a mind to consume and dispose of a received - as opposed to created - object that might be disposable, I would test the object with `obj as IDisposable`, which would not be fooled by another layer of inheritance. Of course, this is terrible code anyways - to dispose of an object that you didn't create that may or may not be disposable. But this is the weird case we are talking about, no? – Keith Payne Sep 12 '13 at 19:14
  • @Servy _to have an interface that defines every single aspect of it's behavior bar disposal_ I think that this is quite common with types that are part of the same code. – Keith Payne Sep 12 '13 at 19:17
  • 1
    @Jon @Servy @Keith This discussion makes no sense. If some library method takes an `IWhatever : IDisposable` as a parameter, whether it disposes it or not, there's no way you could even write a caller to call it with an `IWhateverMinusDisposable` object. So how could that caller be "engineered" in any other way? – Dax Fohl Sep 12 '13 at 19:21
  • 1
    @Jon I disagree, at least in the general case. A `Stream` will, on occasion, be disposed of by the recipient, and in those cases it's clearly documented. In many cases it will not. For an `IEnumerator` you expect whoever created it to be responsible for disposing of it, for a `Task`, you generally expect nobody to dispose of it, for a `DataContext` you'd generally expect the creator to be disposing of it. For a `Bitmap`, like stream, you'll have some of both, so documenting both is important. If you don't control the type you can't force it to implement an additional interface, as I said. – Servy Sep 12 '13 at 19:25
  • @DaxFohl I am thinking of `class disposablefoo : IWhatever, IDispoable { }` versus `class foo : IWhatever { }` with `class f { void Bar(IWhatever d) { } void Dispose() { if (d is IDisposable) d.Dispose(); } }` – Keith Payne Sep 12 '13 at 19:34
  • @KeithPayne: If you make runtime type checks then of course all bets are off. However, the idea is not to resist active meddlers but to protect unknowing users from making an honest mistake. I see that not as a weird case but as good library design. – Jon Sep 12 '13 at 19:45
  • @DaxFohl: As I mentioned above, the idea is to help your users avoid bugs (from the viewpoint of a library writer -- I did not make that explicit, and I now see that was a mistake). Considering library code is reversing this situation (libraries are debugged, users of libraries not as much). You are right in the general case. – Jon Sep 12 '13 at 19:47
  • @Servy: A `Stream` that will be disposed by the creator is practically useless because the recipient would have to test every access for `ObjectDisposedException`, no? An `IEnumerator` is **definitely** supposed to be disposed by the recipient, not by the creator (see expansion [here](http://msdn.microsoft.com/en-us/library/aa664754(v=vs.71).aspx), what did you have in mind?). `Task` can never be safely disposed by the creator because it doesn't know if outside code is waiting on it (see [here](http://blogs.msdn.com/b/pfxteam/archive/2012/03/25/10287435.aspx)). In short, I disagree. – Jon Sep 12 '13 at 19:53
  • 1
    @Jon It is the responsibility of the caller to not dispose of an object that it has given to another object if that other object still needs it. In the vast majority of cases that other object, or the scope in which it's using the provided disposable object (often a single method; not the object's lifetime) is less than the context of the caller, thus this is easy to handle. If I give someone a `Stream` to access I may want to continue using it after they're done. That can't happen if they have already disposed it. `Task` shouldn't be disposed explicitly by anyone, including the recipient. – Servy Sep 12 '13 at 19:57
  • @Jon You also still haven't touched the fact that your proposal isn't even *possible* in a huge percentage of cases, thus saying it should be done in all cases is clearly impossible. – Servy Sep 12 '13 at 19:58
  • @Servy: If you want to continue using the `Stream` afterwards then IMO *in general* you should be proxying it instead of giving it out. `Task` should be disposed by the final recipient if possible, but unless it was waited on the impact of not doing so is nil and pretty small even if it was. Anyway, I have the feeling that we 're mostly having a comm breakdown here (I am thinking about out parameters, you are probably thinking about in) and this is not a good medium for discussions. Let's let it go. – Jon Sep 12 '13 at 20:05
4

Make sure to Dispose the items before they lose scope.

The best way is to use using statement, or you can manually call Dispose, but before it loses scope and becomes eligible for garbage collection.

You may see: CA2000: Dispose objects before losing scope

If a disposable object is not explicitly disposed before all references to it are out of scope, the object will be disposed at some indeterminate time when the garbage collector runs the finalizer of the object. Because an exceptional event might occur that will prevent the finalizer of the object from running, the object should be explicitly disposed instead.

yoozer8
  • 7,361
  • 7
  • 58
  • 93
Habib
  • 219,104
  • 29
  • 407
  • 436
1

As a general rule, yes. The easiest way to do this is usually with the using statement.

Tim S.
  • 55,448
  • 7
  • 96
  • 122
1

Almost always yes, but here is an example of when disposing something borrowed can cause problems. I created a disposable repository with two constructors, one that takes a DbContext, and another default constructor that creates the DbContext itself. I ran into instances when my repository would be disposed, triggering the disposal of the DbContext (because I told it to), and that would sometimes cause problems because I still needed the DbContext that had been passed in elsewhere in the code.

In this case, the repository creates the DbContext and must take responsibility for it and dispose of it because no other code can, but when the DbContext is passed in by some other code then disposing of the DbContext should be the responsibility of the code that created it.

Jeremy Cook
  • 20,840
  • 9
  • 71
  • 77
0

Not mandatory in all scenarios, but its a good practice to dispose an object. If you dont want to do it for each object, use using {} on which disposable is implemented.. It will take care of diposing the object when the code block is over.