views:

544

answers:

10

I can't believe I'm still confused about this but, any way, lets finally nail it:

I have a class that overrides OnPaint to do some drawing. To speed things up, I create the pens, brushes etc before hand, in the constructor, so that OnPaint does not need to keep creating and disposing them.

Now, I make sure that I always dispose of such objects, but I have the feeling I don't need to because, despite the fact they implement IDisposable, they're managed objects.

Is this correct?


Thanks for all the answers, the issue has certainly been nailed.
I'm glad I've been vigilant in always using 'using' so that I don't need to go through all my code checking. I just wanted to be clear that I wasn't being a pointless user.

As an aside, I did have a strange situation, recently, where I had to replace a using block and manually call dispose! I'll dig that out and create a new question.

+6  A: 

You need to dispose them.

Managed object manage their memory by themselves. But memory isn't the only resource that an object uses. Dispose() is intended to release the other resources.

James Curran
+1  A: 

No, IDisposable is for managed objects that use unmanaged resources. As a rule, you should always dispose of them when finished.

mbeckish
+36  A: 

It is not correct. You need to dispose objects that implement IDisposable. That is why they implement IDisposable - to specify the fact that they wrap (directly or indirectly) unmanaged resources.

In this case, the unmanaged resource is a GDI handle, and if you fail to dispose them when you are actually done with them, you will leak those handles. Now these particular objects have finalizers that will cause the resources to be released when the GC kicks in, but you have no way of knowing when that will happen. It might be 10 seconds from now, it might be 10 days from now; if your application does not generate sufficient memory pressure to cause the GC to kick in and run the finalizers on those brushes/pens/fonts/etc., you can end up starving the OS of GDI resources before the GC ever realizes what's going on.

Additionally, you have no guarantee that every unmanaged wrapper actually implements a finalizer. The .NET Framework itself is pretty consistent in the sense that classes implementing IDisposable implement it with the correct pattern, but it is completely possible for some other class to have a broken implementation that does not include a finalizer and therefore does not clean up properly unless Dispose is called explicitly on it. In general, the purpose of IDisposable is that you are not supposed to know or care about the specific implementation details; rather, if it's disposable, then you dispose of it, period.

Moral of the story: Always dispose IDisposable objects. If your class "owns" objects that are IDisposable, then it should implement IDisposable itself.

Aaronaught
+1  A: 

You really need to look up the documentation for the brushes, pens etc.

If they aren't using unmanaged resources, you may not have to call Dispose. But the using/Dispose pattern is sometimes "misused". As an example, consider the ASP.NET MVC framework. Here you can write something like:

using(Html.BeginForm(...)){
  ///HTML input fields etc.
}

When Html.BeginForm(...) is called, a FORM tag will be output. When the using-statement ends, Dispose will be called on the object returned from Html.BeginForm(...). Calling Dispose causes the end FORM tag to be rendered. In this way the compiler will actually enforce the pairing of FORM tags, so you won't forget the closing tag.

Rune
+2  A: 

Have you profiled this to see if Creating & Disposing these objects really is a problem? I don't think it is.

You make things a lot easier for yourself and certainly less error prone by just following the create-in-a-using-block pattern.

If you do want to create them once, then also implement IDisposable on your owning class and iterate the Dispose over your owned objects. No need for a destructor (finalizer).

There is almost no cost on doing this to objects that don't actually need Dispose, but there is a big cost if you forget Dispose on an object that does need it.

Henk Holterman
+1  A: 

No, Pens and Brushes are not fully managed objects.

They contain a handle to an unmanaged resource, i.e. the corresponding GDI object in the underlying graphics system. (Not certain about the exact terminology here...)

If you don't dispose them, the handles will not be released until the objects are finalised by the garbage collector, and there is no guarantee that it will happen soon, or at all.

Guffa
+3  A: 

I wrote a GDI+ diagramming component which used lots of pens and brushes. I created them and disposed them in the block of code that was doing the drawing and performance was never an issue. Better that then having a long lived handle hanging around in the OS IMHO.

James Westgate
+1  A: 

No that is wrong. I agree with Aaronaught.

In addition, Microsoft recommends, in a mid 2003 webcast that Don Box presented, that every .Net developer should dispose of their own objects, whether managed or unmanaged, as this improves code performance by anything upto 20%. If done right it can be a substantial performance improvement. So its a core skill that every .net developer needs to know and use.

scope_creep
+1  A: 

There is a marked irony in your approach. By pre-creating the pens/brushes, you are exactly creating the problem that Dispose() is trying to solve. Those GDI objects will be around longer, just like they would be when you don't call Dispose(). It is actually worse, they'll be around at least until the form is closed.

They are probably around long enough to get promoted to generation #2. The garbage collector doesn't do a gen#2 collection very often, it is now more important to call Dispose() on them. Do so by moving the Dispose() method of the form from the Designer.cs file to your form.cs file and add the Dispose calls.

But, do this the Right Way. Pens and brushes are very cheap objects. Create them when you need them, in the Paint event. And use the using statement so they'll get disposed right away. Use the Stopwatch class to re-assure yourself this doesn't actually cause any slowdown.

Hans Passant
+1  A: 

Others have alluded to "using" blocks for GDI objects - here's a code example:

using( var bm = new Bitmap() )
using( var brush = new Brush() )
{

   // code that uses the GDI objects goes here
   ...

} // objects are automatically disposed here, even if there's an exception

Note that you can have as many "using" lines as you want for a single code block.

I think this is a nice, clean way to deal with Disposable objects.

Tom Bushell