This is why I am starting to loathe programming

[leppie]

Perfect example of stupid code... But thanks, tomorrow I will add a dispose for each of our 400 or so in the app :)

xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth Edition

[Adriaan Davel]

Why not rather change the using construct to accomodate classes that do not implement IDisposable? Implementing IDisposable should indicate that disposing is required... (I also implement IDisposable just to be bale to use the using construct, but I'm usually unconfortable while doing it :) )

____________________________________________________________ Be brave little warrior, be VERY brave

[Fabio Franco]

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.

[Fabio Franco]

leppie wrote:

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.'

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. Just do a little experiment and see for yourself: public partial class Form1 : Form { public Form1() { InitializeComponent(); } private void button1_Click(object sender, EventArgs e) { Form frm = new Form(); frm.ShowDialog(); //frm.Dispose(); //GC.Collect(); } private void button2_Click(object sender, EventArgs e) { 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); } } } You'll notice that the only way to keep the handle count to a minimum, is through calling Dispose method. If you uncomment GC.Collect() and comment Dispose(), it will keep the handle count from growing indefinitely, but won't stay as small as when calling Dispose() and commenting GC.Collect()

[leppie]

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 Edition

[Fabio Franco]

leppie wrote:

If you implement IDisposable incorrectly, and do not call it from the finalizer if not already disposed, you have stupid code.

Well, then whoever from Microsoft implemented the Form class is stupid. In fact, MSFT doesn't even recommend anyone to implement the Finalizer explicitly. Blindly relying on how someone impliments a finalizer or anything is even more stupid.

leppie wrote:

I am not even going to bother.

I will take that as someone who is not willing to be wrong.

leppie wrote:

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.

Well, then you have never experienced running out of handles. Just because you've not ran into a problem, it doesn't mean it can't happen. Beleive me, it does happen.

[Fabio Franco]

peterchen wrote:

However, you may run out of unmanaged resources, and the GC idnling happily because it doesn't even know those tiny .NET objects hold on to huge unmanaged resource hogs.

Exactly, I've seen that happen for Window Handles. Calling Dispose would have avoided this problem and the huge amount of time trying to pinpoint where the leak was happening.

[Fabio Franco]

leppie wrote:

peterchen wrote: You misspelled "if". No, I meant 'when'. Dispose should always be called from a finalizer.

There is no telling that the GC will run even during a lifetime of the application, you can run into problems before that happens. I agree with the 'if'. This when the finalizer calls the Dispose method correctly, which you can never tell if the code isn't yours.

[gnop]

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.

[Fabio Franco]

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.

[leonej_dt]

I 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.

[Chris Trelawny Ross]

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 a using block and which could safely be used without one. Personally, I'm glad that only things that need to be disposed implement IDisposable.

[CurtainDog]

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.

[CurtainDog]

Don't take it too personally, sometimes being right is an ends in itself.

[Andrew Rissing]

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 { }
        }

[xenry]

Well this is actually not quite true, as a class that implements IDisposable is not required to have a finalizer.

[gnop]

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.

[Fabio Franco]

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

[sgorozco]

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

[Andrew Rissing]

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.