Click here to Skip to main content
15,885,767 members
Please Sign up or sign in to vote.
4.56/5 (8 votes)
See more:
I'm having a memory leak problem, and I'm wondering if anyone can tell me what I'm doing wrong (or what Microsoft bug I missed). Below is a sample application that demonstrates the problem. Call TestCollectTimer.Test() to run the sample.

The problem is, no matter how many "MyTimerData" are created, or how many times GC.Collect() is called, the finalizer of MyTimerData is never called until the application shuts down.

class TestCollectTimer
{
    public static void Test()
    {
        for (int index_A = 0; index_A < 100000; index_A++)
        {
            MyTimerData mtd = new MyTimerData();
            mtd = null;
        }

        GC.Collect();
        Thread.Sleep(2000);
        GC.Collect();

        Form f = new Form();
        f.ShowDialog();
    }
}

class MyTimerData
{
    public System.Threading.Timer m_timer;

    public MyTimerData()
    {
        this.m_timer = new System.Threading.Timer(
            new System.Threading.TimerCallback(this.TimerCall),
            null,
            System.Threading.Timeout.Infinite,
            System.Threading.Timeout.Infinite);
    }

    ~MyTimerData()
    {
        MessageBox.Show("Collect My Timer Data");
    }

    public void TimerCall(object o) { }
}


Thank you for your help.
Posted
Comments
Sergey Alexandrovich Kryukov 22-Apr-11 20:58pm    
Even though this code would not have practical sense, I vote 5 for this very interesting example of memory leak in an application for the managed platform.
--SA
Silent Winter 23-Apr-11 16:25pm    
Thank you SAKryukov. This example was not intended to have practical sense. It is my test application to reproduce a problem in a larger application.
Oleksandr Kulchytskyi 31-May-12 9:24am    
What about m_timer.Dispose() method?)
Read MSDN carefully) As far as you must know,in all classes which implement IDisposable must you must manually invoke Dispose() or incorporate it in statement using(m_timer) { ... }. This rule relate to classes which havn't Finalizators.

Try it this way and it'll work.

The method must be static...
Conversely, if you use a non static method and you reference it using this.NameMethod, the System.Threading.TimerCallback will keep a reference to each MyTimerData object. So, despite your mtd=null instruction, it won't be free until the closing of the applicaton.

Your example is a circular reference:
mtd => m_timer => new System.Threading.TimerCallback(...) => mtd.TimerCall(o){}

here's the working code:

MSIL
class TestCollectTimer
{
    public static void Test()
    {
        for (int index_A = 0; index_A < 100000; index_A++)
        {
            MyTimerData mtd = new MyTimerData();
            mtd = null;
        }

        GC.Collect();
        Thread.Sleep(2000);
        GC.Collect();

        Form f = new Form();
        f.ShowDialog();
    }
}

class MyTimerData
{
    public System.Threading.Timer m_timer;

    public MyTimerData()
    {
        this.m_timer = new System.Threading.Timer(
            new System.Threading.TimerCallback(TimerCall),
            null,
            System.Threading.Timeout.Infinite,
            System.Threading.Timeout.Infinite);
    }

    ~MyTimerData()
    {
        MessageBox.Show("Collect My Timer Data");
    }

    public static void TimerCall(object o) { }
}
 
Share this answer
 
v4
Comments
Sergey Alexandrovich Kryukov 22-Apr-11 20:55pm    
Great answer for a tricky situation, 5+++++++++.

I want to add that changing the timer method to static should no be considered as a tool used to understand the effect of delaying garbage collection (actually, this could serve as a brilliant example of real memory leak in the managed platform, which is incorrectly beleived to be impossible by many), but it should not be considered as a practical advice.

The whole idea if MyTimerDataClass makes no practical sense. Also, in real project the number of timers should be reduced to... zero. Yes, timers are best avoided in favor of thread.

--SA
Bruno Tagliapietra 23-Apr-11 3:10am    
Thanks. But if you wanna use timers and you're dealing with a winform application, the best choice would be System.Windows.Forms.Timer, just because it runs on the UI thread.
Silent Winter 23-Apr-11 16:29pm    
I chose a WinForm application because I'm more familiar with them than command line applications. This timer was not intended to run on a UI thread. I apologize for any confusion.
Sergey Alexandrovich Kryukov 23-Apr-11 21:37pm    
There is no confusion at all; also, please see my response to a comment by orsobrown.
--SA
Sergey Alexandrovich Kryukov 23-Apr-11 21:36pm    
This kind of timer is unfortunately probably the worst one, often blamed for poor accuracy. Running the handler is not a problem at all, Invoke and BeginInvoke is a way to go.
There are very few situations where any timer should be used and preferred over thread.
--SA
Edited: Anyway, the problem is that this is not a good way to give the timer his data. You shouldn't put the timer inside the class that contains the data.

Something like this would be better

MSIL
class TestCollectTimer
    {
        public static void Test()
        {
           MyTimerData data = new MyTimerData();
           data.MyData = 42; //for example...

           System.Threading.Timer myTimer = 
           new System.Threading.Timer(
                new System.Threading.TimerCallback(TimerCall),
                data, //here you pass parameters to the timer
                System.Threading.Timeout.Infinite,
                System.Threading.Timeout.Infinite);
            }
        }
        public static void TimerCall(object o)
        {
            MyTimerData data = (MyTimerData)o;
            //do what you want
        }
    }

    class MyTimerData
    { 
        public int MyData {get; set;}

        ~MyTimerData()
        {
            //...finalize if you have to...
        }
    }
 
Share this answer
 
v5
Comments
Daniel Grunwald 23-Apr-11 12:07pm    
A System.Threading.Timer starts automatically when the constructor is called.
If you add a "m_timer.Start()" call to your WinForms code, you'll find that the same memory leak will occur.
Bruno Tagliapietra 23-Apr-11 20:45pm    
That's true. So this is not a solution... Ok I'm changing it.
The garbage collector exists to give the illusion of infinite memory:
You keep allocating and allocating, and the GC will automatically free memory that it knows will never be used again.

However, in your example, all those objects will be used again - they're needed on the next TimerCall().
A timer event handler cannot be garbage collected because it is still used on the next tick of the timer!
You can think of a global list that contains all active timers - a timer will never be garbage collected while it is active. If it worked any other way, the semantics of your program (how many timer calls you get) would depend on when exactly the GC runs - which is obviously undesirable.

To solve the issue, you have to stop the timer (timer.Dispose()) - this will remove it from the list of active timers, and will allow the GC to collect the timer, the event handler, and related objects.

EDIT: actually, the above answer applies only to System.Timers.Timer and System.Windows.Forms.Timer.

System.Threading.Timer has the strange property that it does not keep itself alive, but only keeps the delegate alive. If there's a reference from the delegate back to the timer, this has the same effect as keeping the timer itself alive. Otherwise, the timer will stop sometime after the timer object became unreachable (so you still can get the undesirable behavior).

In your example, there is the reference chain:
static list of timer callbacks => _TimerCallback => TimerCallback => MyTimerData => Timer

The timer would stop itself in its finalizer, but it can't be garbage collected because it still is reachable, from the static entry that it created itself.
This is a classical leak situation in the .NET world: a static reference is not nulled out by the finalizer, because the finalizer doesn't run, because the static reference isn't nulled out yet. The GC can't look into the future, so doesn't know that the finalizer would remove the last reference to the object.

That said, if Microsoft wanted to fix this leak (which they won't, since it could break programs that depend on such 'unreachable' timers to perform their periodic work), they could now do it, as .NET 4.0 introduced a new type of GC handle that allows a program to communicate this scenerio to the GC:
System.Runtime.CompilerServices.DependentHandle (unfortunately it's internal; publically usable only via the ConditionalWeakTable[^] class). This is an implementation of ephemerons[^] in the .NET GC.
 
Share this answer
 
v4
Comments
Silent Winter 23-Apr-11 16:50pm    
I disagree with the statement "a timer will never be garbage collected while it is active". If the timer pointed to a third class's method, the third class would be collected - same as MyTimerData. I suspect that there is a global list - of timer delegates. Destroying the timer would remove the delegate & free the listener. I would need to see Microsoft's code to confirm this.
Daniel Grunwald 23-Apr-11 17:03pm    
Yes, it depends on the timer implementation. System.Threading.Timer in .NET 4.0 indeed looks like it keeps only the delegate alive, so breaking the reference chain from the delegate to the timer is sufficient to get the timer object collected (which will stop the timer in its finalizer).
However, System.Timers.Timer and System.Windows.Forms.Timer do keep themselves alive, so those need to be stopped explicitly.
Bruno Tagliapietra 23-Apr-11 20:55pm    
"a timer will never be garbage collected while it is active" -- this sentence is false. You should correct it

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900