How should the clean-up of Timers declared inside the scope of a function be managed?

Question

In the following code, a Timer is declared inside a function, where it also subscribes to the Elapsed event:

    void StartTimer()
    {
        System.Timers.Timer timer = new System.Timers.Timer(1000);
        timer.Elapsed += new System.Timers.ElapsedEventHandler(timer_Elapsed);
        timer.AutoReset = false;
        timer.Start();
    }

Once the function completes, the reference to the Timer is lost (I presume).

Does the Elapsed event get unregistered automatically as the object is destroyed, or if not, can the event be unregistered in the Elapsed event itself:

    void timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
    {
        var timer = (System.Timers.Timer)sender;
        timer.Elapsed -= timer_Elapsed;
    }

Would this approach still allow proper cleanup of the object, or should timers never be declared in the local scope of a function for this reason?

Overcompensation

Answer 1

The rules here are convoluted, you can't see what's going on inside the CLR. Which maintains a list of active Timers, a System.Timers.Timer has a reference in that list that keeps it alive and prevents it from getting garbage-collected. Necessary in your case since your local variable in your StartTimer() method isn't enough to keep it alive.

With AutoReset = false, the CLR removes the timer from the list when it ticks. The only reference left is the sender argument in your Elapsed event handler.

If you don't explicitly re-enable the timer by using the sender, thus putting it back in the CLR queue, then there is no reference left to the Timer object. It will be garbage-collected whenever the GC runs.

Unsubscribing the Elapsed event handler has no effect on this. That's another detail that is very hard to see, your event subscription added a reference to this. In other words, the Timer object actually keeps your outer object alive. Which is of course a Good Thing, you would not want your object getting garbage collected while the timer can still call your Elapsed event handler. If you want the object's life-time not getting extended by the timer then you'll have to do more work. Now it is necessary to explicitly unsubscribe the event handler and stop the timer. Which does require you to keep a reference to the Timer object.

Also keep in mind that if your class implements IDisposable itself then it should also dispose the Timer. Necessary because you would not typically want the Elapsed event handler to run on a disposed object, that tends to trigger ObjectDisposedExceptions. Again a reason to keep the Timer object reference stored in a field of your class. Do beware the very nasty threading race bug that's hidden under the floor mat, the Elapsed event can still run after or while you call the timer's Dispose() method. Interlocking is required to prevent that from crashing your program once a year or a month with a blue moon. Not otherwise different from the normal precautions you have to take when you allow code to run on a worker thread and access shared state.

Summarizing, if you have no further use for the Timer then disposing it in the Elapsed event handler is the logical thing to do. It isn't actually necessary, a timer that's not active doesn't consume system resources, but .NET programmers are usually very uncomfortable about skipping it. Again a threading race is possible, you might dispose a timer that's already disposed, but that doesn't cause trouble.

Diastole

Answer 2

Just dispose the timer:

void timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
{
    var timer = (System.Timers.Timer)sender;
    timer.Dispose(); //here
}

Unhooking the event is not necessary because disposal is, by convention, always a complete disposal.

Btw, all of what you have written there is equivalent to:

await Task.Delay(1000);

Closelipped