Lambdas within Extension methods: Possible memory leak?

Question

I just gave an answer to a quite simple question by using an extension method. But after writing it down i remembered that you can't unsubscribe a lambda from an event handler.

So far no big problem. But how does all this behave within an extension method??

Below is my code snipped again. So can anyone enlighten me, if this will lead to myriads of timers hanging around in memory if you call this extension method multiple times?

I would say no, cause the scope of the timer is limited within this function. So after leaving it no one else has a reference to this object. I'm just a little unsure, cause we're here within a static function in a static class.

public static class LabelExtensions
{
    public static Label BlinkText(this Label label, int duration)
    {
        System.Windows.Forms.Timer timer = new System.Windows.Forms.Timer();

        timer.Interval = duration;
        timer.Tick += (sender, e) =>
            {
                timer.Stop();
                label.Font = new Font(label.Font, label.Font.Style ^ FontStyle.Bold);
            };

        label.Font = new Font(label.Font, label.Font.Style | FontStyle.Bold);
        timer.Start();

        return label;
    }
}

Update

Just to clarify i used System.Windows.Forms.Timer. So from your answers it seems, that especially using this timer class just was the right choice cause it does anything the way as i would expect it in this case. If you try another timer class in this case you can run into trouble as Matthew found out. Also i found a way by using WeakReferences if my objects are staying alive or not.

Update 2

After a little sleep and a little more thinking, i also made another change to my tester (answer below) i just added a GC.Collect() after the last line and set the duration to 10000. After starting the BlinkText() several times i hitted all the time my button2 to get the current state and to force a garbage collection. And as it seems all the timers will be destroyed after calling the Stop() method. So also a garbage collection while my BlinkText is already left and the timer is running doesn't lead to any problems.

So after all your good responses and a little more testing i can happily say that it just does what it should without leaving timers in the memory nor throwing away the timers before they done their work.

Answer 1

Your assumption is correct. timer will be allocated when the method is called and be eligible for garbage collection at the end.

The lamba event handler (assuming it's not referenced elsewhere) will also be eligible for garbage collection.

The fact that this is a static method and/or an extension method doesn't change the basic rules of object reachability.

Answer 2

It seems like that Timer will stay alive until the program exit. I tried to dispose the Label and setting its reference to null, but the Timer was not collected until I exited the program.

Answer 3

Memory leaks are not your problem here. The garbage collector can dispose the timer object. But while the timer is not stopped, there seems to be a reference to the timer, so it is not disposed prematurely. To check this, derive a class MyTimer from Timer, override Dispose(bool), and set a breakpoint. After BlinkText call GC.Collect and GC.WaitForPendingFinalizers. After the second call, the first timer instance is disposed.

Answer 4

I just did a fun test along the lines of what @cornerback84 was saying. I created a form with a label and two buttons. One button bound your extension method, the other bound a forced GC.Collect(). I then removed the timer.Stop() in your event loop. It was really fun to click on the start button several times. The interval of the blink got pretty mixed up.

It does look like there is a Memory/Resource leak here... but then again Timer is a disposable object that is never disposed.

Edit:...

Let the fun begin... this may also depend on the Timer that you use.

• System.Windows.Forms.Timer => this will not collect but will work just fine with the system message pump. If you call .Stop() this object could become eligible to collect.

• System.Timers.Timer => this does not use the message pump. Yout much also call the message pump. This will not collect. If you call .Stop() this object could become eligible to collect.

• System.Threading.Timer => This requires invokeing the message pump. But this will also stop on a GC.Collect() (this version didn't seem to leak)

Answer 5

Your code is correct and will not leak memory or resources but only because you stop the Timer in the eventhandler. If you comment out the //timer.Stop(); it will keep on blinking, even when you do a GC.Collect(); later on.

The Timer allocates a Window to listen for WM_ messages and is somehow anchored by that. When the Timer is stopped (Enabled = false), it releases that Window.
The static or extension method context does not play a role.

Answer 6

You should dispose the Timer explicitly by calling its Dispose method or implicitly by calling Stop after using it, so your implementation is correct and would never cause memory leaks.

Answer 7

After reading all your answers i just found a way to better test it.

I updated my Extension class the following way:

public static class LabelExtensions
{
    public static List<WeakReference> _References = new List<WeakReference>();

    public static Label BlinkText(this Label label, int duration)
    {
        Timer timer = new Timer();

        _References.Add(new WeakReference(timer));

        timer.Interval = duration;
        timer.Tick += (sender, e) =>
        {
            timer.Stop();
            label.Font = new Font(label.Font, label.Font.Style ^ FontStyle.Bold);
        };

        label.Font = new Font(label.Font, label.Font.Style | FontStyle.Bold);
        timer.Start();

        return label;
    }
}

Also i created a second button, a second label and added the following code to the click event:

    private void button2_Click(object sender, EventArgs e)
    {
        StringBuilder sb = new StringBuilder();

        for (int i = 0; i < LabelExtensions._References.Count; i++)
        {
            var wr = LabelExtensions._References[i];
            sb.AppendLine(i + " alive: " + wr.IsAlive);
        }

        label2.Text = sb.ToString();
    }

Now i just pushed the first button several times to let the first label blink. Then i pressed my second button and got a list where i can see if my timers are still alive. At a first click they all got a true. But then i hit my first button several times again and when i updated my status message i saw that the first items are already got a false in IsAlive. So i can safely say that this function doesn't lead to any memory problems.