Click here to Skip to main content
Rate this: bad
good
Please Sign up or sign in to vote.
See more: C#
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 22-Apr-11 11:31am
Comments
SAKryukov at 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 at 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 at 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.
Rate this: bad
good
Please Sign up or sign in to vote.

Solution 1

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:
 
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) { }
}
  Permalink  
v4
Comments
SAKryukov at 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
orsobrown at 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 at 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.
SAKryukov at 23-Apr-11 21:37pm
   
There is no confusion at all; also, please see my response to a comment by orsobrown.
--SA
SAKryukov at 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
Sandeep Mewara at 23-Apr-11 0:30am
   
Comment from OP:
Orsobrown, thank you for your feedback, but something still doesn't make sense.
 
My circular reference should be irrelevant. From what I understand, .NET searches the object tree for used objects. Since each MyTimerData is off of the main graph (/tree/whatever), it should be identified for collection. It should not matter that it references itself.
 
Your solution does fix the problem, but does not do what I need. I need to be able to access the object's members. If the called method is static, I lose the "this" pointer. I may end up making a middle class:
mtd -> m_timer -> TimerCallback -> MiddleClass -> WeakReference -> mtd.
But, that just seems wrong. (ugly)
orsobrown at 23-Apr-11 3:04am
   
Sorry, I wasn't clear enough, the matter is only with events and delegates.
GC can deal correctly with most of circular references (althought it should be better to avoid them whenever possible, that means: first declare static, otherwise make it instance-dependent).
However, it's a known fact that the GC has trouble sometimes in circular references involving events and delegates.
I wish I'd have a little more time to spend on this at the moment... Probably we should have a look to the ildasm of a simplified version of that code.
Daniel Grunwald at 23-Apr-11 11:56am
   
The GC can handle circular references just fine, even with events and delegates.
The "event problem" only occurs when you have a long-lived object as events source, and a short-lived object as event handler: the delegate will keep the event handler alive as long as the event source lives.
In the worst case (event source lives forever, e.g. static event), the event handlers will never be garbage collected.
However, if the event source has a short lifetime, you do not need to unregister event handlers.
Silent Winter at 23-Apr-11 16:35pm
   
Daniel Grunwald and orsobrown are in disagreement... I currently agree with Daniel Grunwald. But, if either of you know where you got your information; I would appreciate having a link there.
EdMan196 at 24-Apr-11 6:39am
   
From reading them it appears that they are not actually in disagreement but that orsobrown took a more general view but Daniel Grunwald has explained in detail the specifics of why the general problem may occur. Unless I'm missing something the contradiction is only due to over generalisation on orsobrown's part but we can't blame him for that as it was a perfectly good and obvious conclusion to make.
Sandeep Mewara at 23-Apr-11 0:31am
   
My 5 for effort.
Kim Togo at 23-Apr-11 2:43am
   
My 5. This applies also to other timers a well?
System.Timers.Timer and System.Windows.Forms.Timer.
Daniel Grunwald at 23-Apr-11 17:19pm
   
This only applies to System.Threading.Timer. The other timers keep themselves alive until explicitly stopped.
Daniel Grunwald at 23-Apr-11 17:18pm
   
The reference chain here is actually:
static list of timer callbacks (within .NET runtime) => new System.Threading.TimerCallback(...) => mtd.TimerCall(o){} => mtd => m_timer
If the timer would get garbage collected, it would deregister the callback from the static list. But it can't get garbage collected, since it still is reachable from a GC root. The GC can't look into finalizers to see that they would remove the last remaining reference.
 
So in a sense, this is a "cycle" that the GC can't deal with - but that's because the GC can't look into the future to see that if it collected an object that is still reachable, it would become unreachable.
 
But your solution reads as if the GC couldn't deal with cycles of references - that's giving the wrong impression, as the GC can handle those fine. One only needs to be aware that the whole cycle will live as long as the longest-lived object within the cycle.
Rate this: bad
good
Please Sign up or sign in to vote.

Solution 3

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
 
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...
        }
    }
  Permalink  
v5
Comments
Daniel Grunwald at 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.
orsobrown at 23-Apr-11 20:45pm
   
That's true. So this is not a solution... Ok I'm changing it.
Rate this: bad
good
Please Sign up or sign in to vote.

Solution 4

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.
  Permalink  
v4
Comments
Silent Winter at 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 at 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.
orsobrown at 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)

  Print Answers RSS
0 Sergey Alexandrovich Kryukov 378
1 OriginalGriff 230
2 DamithSL 200
3 Zoltán Zörgő 149
4 Dave Kreskowiak 120
0 OriginalGriff 7,575
1 DamithSL 5,529
2 Sergey Alexandrovich Kryukov 5,279
3 Maciej Los 4,961
4 Kornfeld Eliyahu Peter 4,539


Advertise | Privacy | Mobile
Web01 | 2.8.141223.1 | Last Updated 24 Apr 2011
Copyright © CodeProject, 1999-2014
All Rights Reserved. Terms of Service
Layout: fixed | fluid

CodeProject, 503-250 Ferrand Drive Toronto Ontario, M3C 3G8 Canada +1 416-849-8900 x 100