7

In a class that implements IDisposable, when is it reasonable to check if the object has been disposed and throw ObjectDisposedException if it has? In all public methods and properties (except Dispose)? Sometimes? Never?

Johann Gerell
  • 24,991
  • 10
  • 72
  • 122
  • Would you not simply wrap the object in a using statement? Thus the object being disposed when not needed automatically? – Darren Young May 27 '11 at 09:39
  • 2
    @Darren: Then you're assuming that a **user** of my disposable type behaves correctly. Seldom the case! :-) – Johann Gerell May 27 '11 at 09:40
  • 1
    @Lazarus: GC has *nothing* to do with `IDisposable`/`Dispose`. The GC collects unreferenced objects; it doesn't dispose anything. – LukeH May 27 '11 at 09:44
  • ok nice one.. so if anything goes wrong in dispose then how you will come to know whether object is disposed or not, is that exactly what you are looking for? – Deepesh May 27 '11 at 09:45
  • 2
    @Lazarus: I think you misunderstand what dispose means. Dispose is nothing more than a method in an interface a class implements. It really has nothing to do with the GC or the CLR. – Daniel Hilgarth May 27 '11 at 09:45
  • All - Don't know where my brain was this morning... obviously not enough coffee. I feel very sheepish! :) – Lazarus May 27 '11 at 10:28
  • @Johann Gerell: Please don't forget to accept an answer if any helped you. – Daniel Hilgarth May 30 '11 at 16:53

5 Answers5

4

You should implement that check only in methods that don't work on a disposed object.

For example:
If your class closes the database connections or file handles in Dispose, all methods that need those database connections or file handles need to check if the instance is already exposed.

Daniel Hilgarth
  • 171,043
  • 40
  • 335
  • 443
2

The only thing that is really specified is that public void Dispose() itself should not throw anything.

And any method that needs the (un)managed resources should throw.

That leaves, in my mind, only a few disputable cases:

  • IsOpen, IsDisposed: I would not throw
  • other IsSomeStatus: it depends.
  • Length, Count, Position : I don't think a closed Stream has a Length, so throw

It becomes more difficult when the class combines (unrelated) functions like a Stream and a Collection. We just shouldn't do that.

H H
  • 263,252
  • 30
  • 330
  • 514
2

If an object supports IsDisposed, that method itself should never throw; it would be proper for many other methods to throw if IsDisposed returns true, but the exception should be generated by those methods rather than by IsDisposed. One might have a utility method AssertNotDisposed which would throw if an object is disposed, but such behavior would be expected from a method with that name.

Otherwise, I would suggest that there are many cases where it's useful to have an object hold IDisposable object, and be able to Dispose the inner object while maintaining useful state. For example, an object whose function is to show and maintain a modeless dialog box to get information from a user might usefully keep a copy of the contents of the fields even after the box is closed. Such an object should provide a "Close" method which will Dispose the inner Disposable objects but maintain useful state. While it could also have a Dispose method which would call Close but also set a "NoLongerValid" flag that would cause field properties to throw, I don't think that would really add any value.

I will grant that many cases where an object can hold useful state after it has been disposed are indicative of a class which should perhaps be split. For example, the Font class should perhaps be split into a non-disposable FontInfo class (holding a description of a font, but not a GDI handle) and an IDisposable ReadyFont class (inheriting FontInfo, and encapsulating a GDI font object). Routines that use a font could check whether the object they were given was a FontInfo or a ReadyFont; in the former case, they could create a GDI font, use it, and release it; in the latter case, they could use the ReadyFont's GDI font object and release it. The creator of a ReadyFont would then be responsible for ensuring its cleanup.

As it is, I don't know if the system will try to use the GDI object associated with a control's Font property when rendering the control, but I know that it doesn't squawk if the Font is Disposed (even if it's Disposed before assigning it to the Font property!). Controls are certainly capable of creating new GDI fonts if necessary; I don't know whether they always create a new GDI font or if they only do so if the old one has been disposed. The former behavior would seem to be more performant, but unless coded carefully could cause problems if one thread tried to Dispose a Font while another thread was using it.

supercat
  • 77,689
  • 9
  • 166
  • 211
1

As you say, I would implement this check in all public methods and properties except Dispose and IsDisposed. This is the standard pattern to prevent a developer from using your type in the mistaken impression that it's still valid.

HTTP 410
  • 17,300
  • 12
  • 76
  • 127
1

Using a disposed object is a programming error you want to find as fast as possible.

The more places you check it the faster you will find the error.
You should definitely check it where a disposed object will cause some other kind of exception (i.e. null pointer) to not confuse the user.

In other places it depends on the application if it is worth the effort.

adrianm
  • 14,468
  • 5
  • 55
  • 102