dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.36k stars 967 forks source link

Clarification about System.Drawing.Font and Dispose #8823

Open kirsan31 opened 4 years ago

kirsan31 commented 4 years ago

If nothing special here (docs are correct), why Designer never call Font.Dispose then? Otherwise, can we have official clarification (update docs)?

Ups, sorry about api-suggestion label - my bad :( Pls remove it.

weltkante commented 4 years ago

Fonts are dangerous to dispose because they can be shared, if you want desterminstic disposal you need a thourough concept of ownership to be sure its safe to dispose. The designer certainly doesn't have that. As far as I'm aware most of WinForms lets the GC clean up fonts, simply because its not safe to call Dispose when you don't know who else is still using it.

A better design would have been making a shallow clone of the managed font class when assigning it to a form/control, reference counting how many managed references you have to a native font. Then you could have eager disposal without running the risk of releasing the native font too early.

However thats too late to do, as it actually would decrease "memory performance" since you would have a lot more manged font allocations just in order to support deterministic cleanup. (They'd share the same native font but still cost managed allocations.) Personally I don't think its worth it, I never have seen a problem due to Fonts being cleaned up by GC too slow, I don't think its a resource bottleneck.

As it stands the GC is the only safe place to release fonts, manual disposal of Fonts is only safe if you understand the whole program structure and are sure its not shared anymore at point of disposal.

PS: Bitmaps and Icons have the same problem. For Bitmaps it may actually be worth to implement a reference counting scheme and eager disposal, because lazy disposal by GC can keep files locked. Though I guess not high priority since most people have learned to work around file locking by making in-memory copies of the bitmap if waiting for file unlocking by GC is an issue.

kirsan31 commented 4 years ago

@weltkante Thank you. My thoughts on this lie roughly on the same plane. So we definitely need some clarification on this in docs...

RussKie commented 4 years ago

Thank you for the feedback. We don't technically own Font, it belong to .NET Runtime: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

@JeremyKuhne do you know who would be the owner of System.Drawing.Common to pass the feedback to?

ghost commented 4 years ago

Tagging subscribers to this area: @safern, @tannergooding Notify danmosemsft if you want to be subscribed.

safern commented 4 years ago

@RussKie what is the action here? It seems like the question here is why the Designer never calls Dispose on usages of Font?

kirsan31 commented 4 years ago

The docs clearly says:

Use Dispose, when you not need font any more.

But in WinForms, not a designer, no control (and it's right I think :) ) don't calls Dispose of a Font. As @weltkante mentioned above for a good reason. And as you can see, many ppl got confused with this behaviour. I think, this questions (with different answers) are good summaries of this confusion. My opinion: this is not isolated Font/WinForms question, this is about using Font in WinForms specific.

SingleAccretion commented 4 years ago

One course of action could be putting [EditorBrowsable(EditorBrowsableState.Never)] (and updating the docs, obviously) on font.Dispose(), if calling it could lead to these dangerous side-effects and the resources are released by the finalizer anyway.

RussKie commented 4 years ago

what is the action here?

I'd like to see a (better) guidance on when/whether Font objects should be disposed.

Windows Forms constructs a few instances (and we're up for some guidance from our end, @merriemcgaw), but we also get a number of instances provided to us to .NET Runtime (e.g. via SystemFonts). Is it safe to call Dispose on those instances referenced from the Runtime?

I had a look at the Font docs but I came out none the wiser - the example raises the original question - why aren't fonts disposed of? We can say "because Label1 lives in the outer scope", but then why are we constructing Font instances every time event is raised?

I'm finding the Dispose docs somewhat vague too, e.g. what does this mean for a general developer?

After calling Dispose, you must release all references to the Font so the garbage collector can reclaim the memory that the Font was occupying.

JeremyKuhne commented 4 years ago

As to WinForms usage @weltkante called out:

As far as I'm aware most of WinForms lets the GC clean up fonts, simply because its not safe to call Dispose when you don't know who else is still using it.

Yes, we're stuck in WinForms as we don't have any idea when people aren't using them anymore as we expose them in public API.

There is confusion as Font doesn't dispose FontFamily which is IDisposable, but that hits the same case we generally have in WinForms (where System.Drawing originated). You can't dispose it as it hangs publicly off of Font.FontFamily.

The docs wording is a little confusing as @RussKie points out. I presume it was called out for the above reasons.

In general I would say that it is best practice to null out any fields for objects you have disposed. Font has a known, semi-documented need for it, but you should always do this as a matter of course to reduce rooting depth. That should hold true for Font- it should set the FontFamily field to null when it is disposed.

So those are the action items I see:

JeremyKuhne commented 1 year ago

Unfortunately nulling out the field isn't really plausible: https://github.com/dotnet/winforms/pull/8986#issuecomment-1507751031. If someone wants to make a doc clarification about disposal that would be great.

ghost commented 1 year ago

This issue is now marked as "help wanted", and we’re looking for a community volunteer to work on this issue. If we receive no interest in 180 days, we will close the issue. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

kirsan31 commented 1 year ago

If someone wants to make a doc clarification about disposal that would be great.

I really doubt that someone from the outside can adequately and correctly describe all of this in the documentation 🤔