29

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.

Esoteric Screen Name
  • 6,082
  • 4
  • 29
  • 38
Jules
  • 4,319
  • 3
  • 44
  • 72

11 Answers11

44

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
  • 120,909
  • 25
  • 266
  • 342
  • I might call the use of finalizers the "ideal" pattern, but there are many cases where IDisposable objects cannot usefully implement finalizers, e.g. where they use thread-static variables or event subscriptions from unknown objects. Finalizers are run in a different thread context from other code, which would have a hard time accessing the thread-static variables associated with an abandoned IDisposable created on another thread. While it's possible to design events to be unhooked by a finalizer, most events cannot be safely unhooked in such a context. – supercat Oct 18 '11 at 13:11
  • 1
    @supercat: That's a very dangerous design, storing unmanaged resources in thread-local storage. True enough that finalizers will likely run in a different thread, but how can you guarantee that `Dispose` will be called from the same thread that caused the resource to be initialized? I can think of several situations where that wouldn't be the case. Honestly, don't use ThreadStatic for that kind of thing, keep critical resources where you can track them. – Aaronaught Oct 19 '11 at 00:44
  • Items in thread-local storage are often, *in and of themselves*, unmanaged resources; if the code which creates them gives up control without clearing them out, but the thread persists (perhaps in the thread pool), such items may never get cleaned up. Using IDisposable for such things allows them to be managed with "using" blocks, which by their nature will handle the threading properly. – supercat Oct 19 '11 at 04:20
  • As an example, one might wrap the constructor of an IDisposable with code which creates a thread-static List, into which the constructor will copy all IDisposable objects it creates. If the constructor completes normally, the wrapper can destroy the thread-static reference to the list. Otherwise, if the constructor throws, the wrapper can use the thread-static list to clean up the stranded IDisposables that were created by the failed constructor. Not a totally clean pattern, but since neither C# nor vb offers any good way to deal with failed constructors, it's the best I've found. – supercat Oct 19 '11 at 04:31
  • Such a pattern will work to ensure that any IDisposable objects that created in field initializers or base-class constructors will get cleaned up even if a derived-class constructor throws an exception. The only problem with the pattern is that it won't work if the wrapper object isn't Disposed, and there's no way for the wrapper to use a finalizer to perform the cleanup if the proper thread doesn't. – supercat Oct 19 '11 at 04:36
  • @supercat: That sounds like an egregiously bad hack to work around race conditions or other synchronization/re-entrancy problems. If a class constructor is going to store long-lived references to TLS data *as instance data* then it might as well just *use instance data*, or a global resource pool if necessary (whereby the pool itself can detect abandoned resources through weak references). Constructors are single-threaded so there's no reason for them to be referencing ThreadStatic fields at all. – Aaronaught Oct 19 '11 at 15:44
  • It isn't always practical to pass necessary information as parameters to a function, for a variety of reasons. Thread-local storage provides a somewhat icky, but workable, solution. Any code which puts something into thread-local storage, however, has a responsibility to ensure that it will get taken out. In my particular example, the TLS data couldn't originate as instance data because the code generating the data didn't have an instance to store it in, and constructor parameters aren't available to *any* code until after all fields are initialized. – supercat Oct 19 '11 at 18:58
  • The thread-local storage is really only needed during the interval between the wrapper's call to the constructor and the start of the main body of the constructor, but I know of no thread-safe way in C#, other than thread-static variables, to pass data to things like field initializers. Using thread-static variables for such purposes is icky and probably not very performant, but my original point, which is that any code which stores a thread-static reference to anything has a responsibility for cleaning it up, and that a Finalize() method can't handle such cleanup, remains. – supercat Oct 19 '11 at 19:13
10

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
  • 101,701
  • 37
  • 181
  • 258
5

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
  • 922,412
  • 146
  • 1,693
  • 2,536
3

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
  • 11,306
  • 8
  • 61
  • 68
2

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.

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

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

mbeckish
  • 10,485
  • 5
  • 30
  • 55
1

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
  • 8,340
  • 3
  • 34
  • 47
1

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
  • 687,336
  • 108
  • 737
  • 1,005
1

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
  • 4,213
  • 11
  • 35
  • 64
1

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
  • 5,865
  • 4
  • 45
  • 60
0

Although you asked about pens and brushes, Font is a class with some odd quirks. In particular, if one creates a font for the purpose of setting a control's Font property, one remains responsible for disposing of that font--ownership does not transfer to the control--but that responsibility can be carried out by disposing of the font at any time--even as soon as the font is created, before assigning it to the control. It seems Font is a combination of a managed information object and an unmanaged GDI resource, and for some purposes only the former is needed. Weird design--Font should have been two classes.

supercat
  • 77,689
  • 9
  • 166
  • 211