CODEWITHSUNDEEP
c#.netperformancememory-leaksgarbage-collection
Jon
Jon

Reputation: 40032

Is this a memory leak or will garbage collection fix it

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?

Upvotes: 4

Views: 4029

Answers (3)

JaredPar
JaredPar

Reputation: 754585

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();

Upvotes: 5

Jon
Jon

Reputation: 40032

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.

Upvotes: 2

the_joric
the_joric

Reputation: 12226

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);

Upvotes: 2

Related Questions