4

Lets say I have a button that gets clicked and it does this:

public void ButtonClick(object sender, EventArgs e)
{
  System.Timers.Timer NewTimer = new System.Timers.Timer();
  NewTimer.AutoReset = false;
  NewTimer.Elapsed += new ElapsedEventHandler(TimerElapsed);
  NewTimer.Interval = 1000;
  NewTimer.Start(); 
}

public void TimerElapsed(object sender, ElapsedEventArgs e)
{

}

If this button gets clicked 100 times what happens to those instances that have been created? Will garbage collection kick in or does the System.Timers.Timer.Close method need calling and if it does where do you call it from?

Jon
  • 38,814
  • 81
  • 233
  • 382

3 Answers3

5

No this will not cause a memory leak. In fact the way your code is written it's not guaranteed to execute properly. Timers.Timer is really just a wrapper over Threading.Timer and it's explicitly listed as being collectable even if it's currently running.

http://msdn.microsoft.com/en-us/library/system.threading.timer.aspx

Here you keep no reference to it and hence the very next GC could collect it while your form is still running and before the event ever fires

EDIT

The documentation for Timers.Timer appears to be incorrect. The Timer instance will not be collected if it's unreferenced. It will indeed live on 

var timer = new System.Timers.Timer
{
    Interval = 400,
    AutoReset = true
};
timer.Elapsed += (_, __) => Console.WriteLine("Stayin alive (2)...");
timer.Enabled = true;

WeakReference weakTimer = new WeakReference(timer);
timer = null;

for (int i = 0; i < 100; i++)
{
    GC.Collect();
    GC.WaitForPendingFinalizers();
}

Console.WriteLine("Weak Reference: {0}", weakTimer.Target);
Console.ReadKey();
JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
  • I'm not in a form its just or demo purposes. Imagine its a method call in a class which might get fired a lot. When do I close it then? – Jon Feb 21 '12 at 16:32
  • Just did some tests and using Redgate Antz when GC kicked in the timers were left in memory – Jon Feb 21 '12 at 16:46
  • @Jon then either they had already been promoted to generation 2 and it was a gen 1 only GC or they were referenced elsewhere. They are most definitely collectable if they are not referenced. – JaredPar Feb 21 '12 at 16:47
  • They weren't referenced elsewhere it uses the code in the question and I would imagine the profiler would use GC 0 – Jon Feb 21 '12 at 16:52
  • 1
    @JaredPar or they are waiting for their finalizers to run -- a finalizable object needs 2 GCs. – phoog Feb 21 '12 at 16:54
  • 1
    @Jon they're finalizable so they need to wait for their finalizers to run. – phoog Feb 21 '12 at 16:54
  • Done some more testing using Antz and Threading.Timer does get collected but Timers.Timer still persists – Jon Feb 21 '12 at 17:03
  • @JaredPar How do you suppose I keep references to the various times that may get created? – Jon Feb 21 '12 at 18:16
  • @Jon the easiest way is to just add a `List` onto the `Form` and store them there. – JaredPar Feb 21 '12 at 18:36
  • @JaredPar In terms of cleanup I can close the timer on the Elapsed event but how would I remove it from the list as I wouldnt know which timer it was in the list? – Jon Feb 22 '12 at 08:44
  • @JaredPar Please see this question which shows the timers still stick around http://stackoverflow.com/questions/4962172/why-does-a-system-timers-timer-survive-gc-but-not-system-threading-timer – Jon Feb 22 '12 at 09:51
  • @Jon interesting. I ran my own experiment and it looks like the docs are wrong on `Timers.Timer`. Note there are some subtle things you need to change in your code sample in order to be a true GC test (multiple GC's, `WaitForPendingFinalizers, etc ...) but I added them in my version and yep they're still around. – JaredPar Feb 22 '12 at 18:05
  • @Jon it looks like `Timers.Timer` is creating a nasty circular reference issue. There is a strong GC handle in the callback which points to `Timers.Timer`. That in turn holds onto `Threading.Timer` and only the disposing of `THreading.Timer` causes the callback to be removed. Nasty. Will report that – JaredPar Feb 22 '12 at 18:16
  • @JaredPar Who do you report it to and how? – Jon Feb 23 '12 at 09:11
  • @Jon actually as I look more I think it's by design. The docs I found were specifically for `Threading.Timers`. I thought it said for both but now I can find it. Could've just misread it. Still looking into it. – JaredPar Feb 23 '12 at 17:36
2

They will be collected once method is left. TimerElapsed will be either called or not depending on when Timer gets finalized. Most likely it will be dead long before 1 second passed.

When you call Timer.Close() you thus call Timer.Dispose() that de-registers timer from timer queue and in that case TimerElapsed won't be called (of course if it was not called before).

If you leave timer not closed, GC will eventaully call Finalize() that in turn will call Dispose(). But there is not exact knowledge when it will happen :)

See below example, Console.Out.WriteLine("called!!!") will never execute:

using (System.Timers.Timer NewTimer = new System.Timers.Timer())
{
    NewTimer.AutoReset = false;
    ElapsedEventHandler TimerElapsed = (sender, args) => { Console.Out.WriteLine("called!!!"); };
    NewTimer.Elapsed += new ElapsedEventHandler(TimerElapsed);
    NewTimer.Interval = 1000;
    NewTimer.Start();
}

Thread.Sleep(3000);
the_joric
  • 11,986
  • 6
  • 36
  • 57
  • Or once the elapsed event has fired? – Jon Feb 21 '12 at 16:30
  • Can you call Close on the elapsed event? – Jon Feb 21 '12 at 16:43
  • Sure, but if timer is still a method variable, elapsed event may never be fired :). You need to make it a class variable (field). – the_joric Feb 21 '12 at 16:46
  • In the elapsed event could you not just call ((Timer)sender).Close() – Jon Feb 21 '12 at 16:47
  • You could :) But elapsed event code won't be executed in your exapmle (in 99% cases at least). Also calling Close() does not have much sense, because you've set AutoReset to false, that means that event is going to be executed only once. – the_joric Feb 21 '12 at 16:51
  • Why wont my elapsed event get called 99% of the time? – Jon Feb 21 '12 at 17:00
  • Because Timer will get collected before 1 seс elapses. Did you try my code example? – the_joric Feb 21 '12 at 17:01
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/8026/discussion-between-the-joric-and-jon) – the_joric Feb 21 '12 at 17:02
  • I can see in your example why the elapsed event wont occur but I can't see why my wouldnt – Jon Feb 21 '12 at 17:02
  • Please see this question which shows the timers still stick around http://stackoverflow.com/questions/4962172/why-does-a-system-timers-timer-survive-gc-but-not-system-threading-timer – Jon Feb 22 '12 at 09:51
2

After answers by the_joric and JaredPar and running profiler tests which showed timers sticking around after garbage collection kicked in the reason they stuck around was because there is a reference to the event handler sticking around. For a more detailed explanation see this answer.

The real answer is that it is a memory leak unless the timer is closed in the elapsed event handler.

Just goes to show that although I trust the answers on SO (maybe too much) from the great contributors they may be slightly off.

Community
  • 1
  • 1
Jon
  • 38,814
  • 81
  • 233
  • 382