This is why I am starting to loathe programming
-
The Form class is implemented correctly, it inherits from Component which calls Dispose(false) from the Finalize method. The Form class overrides the Dispose(bool disposing) method and has it's own clean up code in there [which in turn calls the base class dispose(false)]. Which is the documented way of implementing IDisposable.
I beleive that too, but leppie is saying we shouldn't call Dispose as the finalizer will always do that for us. In the experiment I proposed, if we don't call Dispose, some handles will be left behind after a few repeated calls of the form's ShowDialog method, even after collection occurs. My point is, call Dispose always and don't rely on finalizer or garbage collection as you can run into trouble. Calling the Form's Dispose method eliminates the extra handles problem, that was my point.
-
It's like the blind leading the blind.... http://stackoverflow.com/questions/2926869/c-do-you-need-to-dispose-of-objects-and-set-them-to-null/2926877#2926877[^] I think I will get similar responses from here too though :sigh:
xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth EditionI loathe programming as well, but for completely different reasons. I loathe programming because of programmers. Programmers are lazy, stupid and don't understand abstractions, even when they leak. Manage resources frustrate me. Manage resource tedious. Me hate manage resources. Me invent garbage collect. Ooga, booga, garbage collect! Some resources can't garbage collect. Cleanup some resources can't automate. Cleanup some resources need intelligence. Same class both managed resources unmanaged resources. Me confused. Ooga, booga, me confused! Programmers deserve to be punished by making them program ERPs in assembly (optimizing CPU cycles, of course) and efficient operating systems in Scheme.
If you can play The Dance of Eternity (Dream Theater), then we shall make a band.
-
OK, let me put it in a slightly different way: If MS had implemented it Piebald's way, how would we be worse off?
Cheers, Vikram. (Got my troika of CCCs!)
If every object supported
IDisposable
, then there would be no easy way to know which really needed it and which didn't - so it would be impossible to know which to wrap in ausing
block and which could safely be used without one. Personally, I'm glad that only things that need to be disposed implementIDisposable
. -
By implementing IDisposable, the class creator explicitely told you to call
Dispose()
. If the documentation of a class said "You need to call Init() before using an instance of this class", would you reply with "Ah, I don#t feel like it today"?Agh! Reality! My Archnemesis![^]
| FoldWithUs! | sighist | µLaunch - program launcher for server core and hyper-v server.If the class creator wants it to be used in a particular way then they can build a factory for it. Up until that point I'll use the class however I want to.
-
It's like the blind leading the blind.... http://stackoverflow.com/questions/2926869/c-do-you-need-to-dispose-of-objects-and-set-them-to-null/2926877#2926877[^] I think I will get similar responses from here too though :sigh:
xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth EditionDon't take it too personally, sometimes being right is an ends in itself.
-
I beleive that too, but leppie is saying we shouldn't call Dispose as the finalizer will always do that for us. In the experiment I proposed, if we don't call Dispose, some handles will be left behind after a few repeated calls of the form's ShowDialog method, even after collection occurs. My point is, call Dispose always and don't rely on finalizer or garbage collection as you can run into trouble. Calling the Form's Dispose method eliminates the extra handles problem, that was my point.
If you're stating that the extra handles are dangling around waiting for the finalizer to kick in, sure you are correct. You're not leaking memory at all, but rather leaving around objects that can be free'd (to use a C++ term) immediately. If you elaborate on your code a little, it'll prove that the finalizer is called just non-deterministically in your original code:
public partial class Form1 : Form { public Form1() { InitializeComponent(); } private void ShowForm() { TestForm frm = new TestForm(); frm.ShowDialog(); //frm.Dispose(); } private void button1\_Click(object sender, EventArgs e) { this.ShowForm(); GC.Collect(); GC.WaitForFullGCComplete(); GC.WaitForPendingFinalizers(); this.Text = HWndCounter.GetWindowHandlesForCurrentProcess().ToString(); } public class HWndCounter { \[DllImport("kernel32.dll")\] private static extern IntPtr GetCurrentProcess(); \[DllImport("user32.dll")\] private static extern uint GetGuiResources(IntPtr hProcess, uint uiFlags); public static int GetWindowHandlesForCurrentProcess() { IntPtr processHandle = GetCurrentProcess(); uint userObjects = GetGuiResources(processHandle, 1); // 1 for User resource, disregard GDI return Convert.ToInt32(userObjects); } } public class TestForm : Form { ~TestForm() { try { Trace.Write("TestForm #" + FormHandle + " finalized.\\r\\n"); } catch { } } private int FormHandle; protected override void OnShown(EventArgs e) { base.OnShown(e); try { FormHandle = this.Handle.ToInt32(); Trace.Write("TestForm #" + FormHandle + " shown.\\r\\n"); } catch { } } protected override void OnClosed(EventArgs e) { base.OnClosed(e); try { Trace.Write("TestForm #" + FormHandle + " closed.\\r\\n"); } catch { } }
-
peterchen wrote:
the class creator explicitely told you to call Dispose().
No, he actually said: 'If you want to cleanup the resources immediately, then call Dispose(), but if you forget or dont want to, I'll do it anyways when the object finalizer is called.'
xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth Edition -
I beleive that too, but leppie is saying we shouldn't call Dispose as the finalizer will always do that for us. In the experiment I proposed, if we don't call Dispose, some handles will be left behind after a few repeated calls of the form's ShowDialog method, even after collection occurs. My point is, call Dispose always and don't rely on finalizer or garbage collection as you can run into trouble. Calling the Form's Dispose method eliminates the extra handles problem, that was my point.
I agree, I always call the dispose method if implemented. I couldn't find the handle leak in your test app. The handles do increase but when a GC collection occurs (for me I had to create about 30 dialogs) the count returns to what it was initially?? This is because the finalize method is executed which in turn calls dispose.
-
If you're stating that the extra handles are dangling around waiting for the finalizer to kick in, sure you are correct. You're not leaking memory at all, but rather leaving around objects that can be free'd (to use a C++ term) immediately. If you elaborate on your code a little, it'll prove that the finalizer is called just non-deterministically in your original code:
public partial class Form1 : Form { public Form1() { InitializeComponent(); } private void ShowForm() { TestForm frm = new TestForm(); frm.ShowDialog(); //frm.Dispose(); } private void button1\_Click(object sender, EventArgs e) { this.ShowForm(); GC.Collect(); GC.WaitForFullGCComplete(); GC.WaitForPendingFinalizers(); this.Text = HWndCounter.GetWindowHandlesForCurrentProcess().ToString(); } public class HWndCounter { \[DllImport("kernel32.dll")\] private static extern IntPtr GetCurrentProcess(); \[DllImport("user32.dll")\] private static extern uint GetGuiResources(IntPtr hProcess, uint uiFlags); public static int GetWindowHandlesForCurrentProcess() { IntPtr processHandle = GetCurrentProcess(); uint userObjects = GetGuiResources(processHandle, 1); // 1 for User resource, disregard GDI return Convert.ToInt32(userObjects); } } public class TestForm : Form { ~TestForm() { try { Trace.Write("TestForm #" + FormHandle + " finalized.\\r\\n"); } catch { } } private int FormHandle; protected override void OnShown(EventArgs e) { base.OnShown(e); try { FormHandle = this.Handle.ToInt32(); Trace.Write("TestForm #" + FormHandle + " shown.\\r\\n"); } catch { } } protected override void OnClosed(EventArgs e) { base.OnClosed(e); try { Trace.Write("TestForm #" + FormHandle + " closed.\\r\\n"); } catch { } }
I get what you're saying, but have you noticed that if you don't call Dispose, even with that solution, one handle gets left behind? Put this: this.Text = HWndCounter.GetWindowHandlesForCurrentProcess().ToString(); before ShowDialog() And then test it commenting and uncommenting the frm.Dispose() without calling Dispose, with your code, I get 42 handles. Calling dispose, I get 41. What it seems to me is that after the finalizer gets called, it still does not free the handle (even calling Dispose method inside the finalizer). I also noticed, that if I call the form's Dispose method from the calling method, the finalizer does not get called. Bottom line is that I never get to reclaim the handle without explicitly Disposing it on the calling method. Even after repeatedly executing this after closing the form: GC.Collect(); GC.WaitForFullGCComplete(); GC.WaitForPendingFinalizers(); I feel there's still something missing I'm not seeing... But to be on the safe side, I still recommend everyone, always call Dispose method if the class implements IDisposable
-
using "using", certifies that the Dispose method gets called on classes that implement IDisposable interface. You don't really need to use it as long as you call Dispose method manually. The most common example is the Window Handle of the Form class. If you use ShowDialog to view a form and don't call the Dispose method, the handle will not be released even when the Form is GCed. GC does not handle unmanaged resources automatically, it does not call Dispose method of objects when they are collected, so yes, it can lead to memory leak. This rarely becomes a problem, but I've ran into this once and it was a pain to pinpoint the leak source. Controls also generate handles, if you have an application that creates lots of controls dinamically it's something you should pay a lots of attention. So, if a class implements IDisposable, call Dispose always, to make sure it releases unmanaged resource just in case it uses any.
Hi Fabio, Sorry, your example is not entirely correct. If you do not call Dispose() on a Form class instance, it will still be disposed (with its underlying window handle properly released) when it's garbage collected. The IDisposable pattern (which is implemented, as far as I know, in all .net Framework classes that hold native resources) takes precisely care of this. The only difference is that you don't know exactly when will the garbage collection occur and since it's triggered by memory allocation pressure, it might take a while for it to actually happen. That's why it's wise to call Dispose(). Cheers! :) Gerardo
-
I get what you're saying, but have you noticed that if you don't call Dispose, even with that solution, one handle gets left behind? Put this: this.Text = HWndCounter.GetWindowHandlesForCurrentProcess().ToString(); before ShowDialog() And then test it commenting and uncommenting the frm.Dispose() without calling Dispose, with your code, I get 42 handles. Calling dispose, I get 41. What it seems to me is that after the finalizer gets called, it still does not free the handle (even calling Dispose method inside the finalizer). I also noticed, that if I call the form's Dispose method from the calling method, the finalizer does not get called. Bottom line is that I never get to reclaim the handle without explicitly Disposing it on the calling method. Even after repeatedly executing this after closing the form: GC.Collect(); GC.WaitForFullGCComplete(); GC.WaitForPendingFinalizers(); I feel there's still something missing I'm not seeing... But to be on the safe side, I still recommend everyone, always call Dispose method if the class implements IDisposable
It's likely the extra handle is for a Font or Image that is statically allocated, but only done so once it was loaded. So, I wouldn't call that a leak. It is evident by the fact that the object is being disposed and/or finalized, but the overall count is not going up (minus the initial value). As for the finalizer not being called, that's on purpose. When you implement the IDisposable pattern, you're supposed to do the following for the public Dispose method:
public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}The last line will suppress the finalizer because of two reasons: 1) It's not needed anymore, the finalizer is meant to call dispose in case it wasn't called. 2) Secondly, objects that need finalizing (as seen with our example) are cleaned up at the framework's whim. This can lead to more memory pressure as an object isn't cleaned up in a timely manner. I whole-heartedly agree that if a class implements IDisposable, that you should call it. But it is important to know the underlying reasons behind that and why the IDiposable pattern works (when coded correctly). Essentially, the pattern is in place to prevent a simple oversight from turning into unusable code.
-
It's likely the extra handle is for a Font or Image that is statically allocated, but only done so once it was loaded. So, I wouldn't call that a leak. It is evident by the fact that the object is being disposed and/or finalized, but the overall count is not going up (minus the initial value). As for the finalizer not being called, that's on purpose. When you implement the IDisposable pattern, you're supposed to do the following for the public Dispose method:
public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}The last line will suppress the finalizer because of two reasons: 1) It's not needed anymore, the finalizer is meant to call dispose in case it wasn't called. 2) Secondly, objects that need finalizing (as seen with our example) are cleaned up at the framework's whim. This can lead to more memory pressure as an object isn't cleaned up in a timely manner. I whole-heartedly agree that if a class implements IDisposable, that you should call it. But it is important to know the underlying reasons behind that and why the IDiposable pattern works (when coded correctly). Essentially, the pattern is in place to prevent a simple oversight from turning into unusable code.
Btw, add the following to the class as well, to help understand the flow of logic:
protected override void Dispose(bool disposing) { base.Dispose(disposing); Trace.Write("TestForm #" + FormHandle + " disposed.\\r\\n"); }
-
Fabio Franco wrote:
Wrong, the Finalizer does not assure the release of unmanaged resources, if there is a Dispose method call it. The Dispose method is not called by the garbage collector.
If you implement IDisposable incorrectly, and do not call it from the finalizer if not already disposed, you have stupid code.
Fabio Franco wrote:
Just do a little experiment and see for yourself:
I am not even going to bother.
Fabio Franco wrote:
If you uncomment GC.Collect() and comment Dispose(), it will keep the handle count from growing indefinitely
It's up to you how you deal with it. My point still stands, you dont have to do it, it is not a MUST DO OR DIE situation.
xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth EditionI agree with Leppie. The IDisposable pattern (when properly implemented) has a safety net that will release native resources even if we didn't call Dispose() or wrapped our instance with a using statement. Garbage collection might take a while to kick in, and yes, it is possible to write an application where garbage collection will never occur, but if you're talking of resource exhaustion, then that means you are constantly creating objects, then THERE is memory pressure and the gc WILL eventually run (unless of course, you are unnecesarily keeping references to objects that should be collected and by doing so, inducing a memory leak). Cheers, Gerardo
-
I agree, I always call the dispose method if implemented. I couldn't find the handle leak in your test app. The handles do increase but when a GC collection occurs (for me I had to create about 30 dialogs) the count returns to what it was initially?? This is because the finalize method is executed which in turn calls dispose.
Commenting and Uncommenting frm.Dispose() makes no difference in handle count? Forcing the collection also won't make a difference?
-
Hi Fabio, Sorry, your example is not entirely correct. If you do not call Dispose() on a Form class instance, it will still be disposed (with its underlying window handle properly released) when it's garbage collected. The IDisposable pattern (which is implemented, as far as I know, in all .net Framework classes that hold native resources) takes precisely care of this. The only difference is that you don't know exactly when will the garbage collection occur and since it's triggered by memory allocation pressure, it might take a while for it to actually happen. That's why it's wise to call Dispose(). Cheers! :) Gerardo
Hi Gerardo, I know that GC can't be predicted, however you can force a collection with GC.Collect() Garbage Collection is no guarantee that Dispose gets called, it all depends on the finalizer implementation. If Dispose correctly releases unmanaged resources and if you call it from the finalizer (good code), then yes, unmanaged resource will be released when a collection occurs, if you don't, Dispose method will never get called and the unmanaged resources will never be reclaimed (not until the process ends). In another discussion in this topic I provide an example of exactly that, where the Window handle is not released even after forcing collection (finalizer gets called, you can see it throug output windows). Only way it did was through explicitly calling Dispose method afer the form was closed, inside the calling method. Regards, Fábio
-
It's likely the extra handle is for a Font or Image that is statically allocated, but only done so once it was loaded. So, I wouldn't call that a leak. It is evident by the fact that the object is being disposed and/or finalized, but the overall count is not going up (minus the initial value). As for the finalizer not being called, that's on purpose. When you implement the IDisposable pattern, you're supposed to do the following for the public Dispose method:
public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}The last line will suppress the finalizer because of two reasons: 1) It's not needed anymore, the finalizer is meant to call dispose in case it wasn't called. 2) Secondly, objects that need finalizing (as seen with our example) are cleaned up at the framework's whim. This can lead to more memory pressure as an object isn't cleaned up in a timely manner. I whole-heartedly agree that if a class implements IDisposable, that you should call it. But it is important to know the underlying reasons behind that and why the IDiposable pattern works (when coded correctly). Essentially, the pattern is in place to prevent a simple oversight from turning into unusable code.
Andrew Rissing wrote:
It's likely the extra handle is for a Font or Image that is statically allocated, but only done so once it was loaded. So, I wouldn't call that a leak. It is evident by the fact that the object is being disposed and/or finalized, but the overall count is not going up (minus the initial value).
Ok, but why does the count comes down when the form's Dispose method is called from the calling method, and not from the finalizer? I mean if it is something static, it should not come down either way right? This is what's boggling me. Thank you for your clarifications, it surely filled a few gaps. Still, there is this thing, bothering me... Did you get to reproduce what I was saying?
-
Hi Gerardo, I know that GC can't be predicted, however you can force a collection with GC.Collect() Garbage Collection is no guarantee that Dispose gets called, it all depends on the finalizer implementation. If Dispose correctly releases unmanaged resources and if you call it from the finalizer (good code), then yes, unmanaged resource will be released when a collection occurs, if you don't, Dispose method will never get called and the unmanaged resources will never be reclaimed (not until the process ends). In another discussion in this topic I provide an example of exactly that, where the Window handle is not released even after forcing collection (finalizer gets called, you can see it throug output windows). Only way it did was through explicitly calling Dispose method afer the form was closed, inside the calling method. Regards, Fábio
Hi Fabio, Yes, you are correct, good code should follow the IDisposable pattern documented by Microsoft, that way you can dispose unmanaged resources early if you want, or leave it to the GC. I'll look at your example, because if what you say is true, then Microsoft is not following its own recommended patterns! Thanks for the reply, Gerardo
-
Hi Fabio, Yes, you are correct, good code should follow the IDisposable pattern documented by Microsoft, that way you can dispose unmanaged resources early if you want, or leave it to the GC. I'll look at your example, because if what you say is true, then Microsoft is not following its own recommended patterns! Thanks for the reply, Gerardo
Thanks Gerardo, yes the point was exactly that, and I prefer not to rely on the Finalizer implementation, simply call Dispose and play the safe side.
-
Andrew Rissing wrote:
It's likely the extra handle is for a Font or Image that is statically allocated, but only done so once it was loaded. So, I wouldn't call that a leak. It is evident by the fact that the object is being disposed and/or finalized, but the overall count is not going up (minus the initial value).
Ok, but why does the count comes down when the form's Dispose method is called from the calling method, and not from the finalizer? I mean if it is something static, it should not come down either way right? This is what's boggling me. Thank you for your clarifications, it surely filled a few gaps. Still, there is this thing, bothering me... Did you get to reproduce what I was saying?
Indeed, I was able to reproduct it. I don't know though if its a memory leak or just something else we aren't aware of. If it was a true memory leak, I would imagine that it's behavior would be repeatable and the allocations would keep rising. Ultimately, one handle is pretty insignificant in the grand scheme of things.
-
Indeed, I was able to reproduct it. I don't know though if its a memory leak or just something else we aren't aware of. If it was a true memory leak, I would imagine that it's behavior would be repeatable and the allocations would keep rising. Ultimately, one handle is pretty insignificant in the grand scheme of things.
Yes, this keeps me thinking that this is just one case, for one form. What else might have this behaviour? and depending on the scenario, this might rise significantly. So I have no doubt that calling Dispose (when you desire to release resources of course) is a very strong should, no matter how the Garbage Collector gets invoked. One theory I have about this behaviour is that one of these handles is kept for performance issues. When Dispose does not get called and the object is collected, the Garbage Collector (or the Finalizer / IDisposable implementation) might think it will be reused (instatiated again at some point of time) and so the handle is kept for reuse with the same form. If you experiment a little more, you'll also notice that another handle increment will happen if you instatiate another Form implementation (besides from TestForm) and don't call Dispose. Same thing will happen for that form. Anyways, this was a nice discussion to have. Regards, Fábio