CODEWITHSUNDEEP
c#.nettimerdisposeanonymous
tronious
tronious

Reputation: 1577

Dispose Timer inside of anonymous method

I have a block of code that will be called relatively often. Prior to it being called I need a 2000ms delay to take place.

The first thing that has come to mind is creating/disposing of a timer every time the method is called.

To accomplish this I'm using a Timer (see code). My question is...any dangers/problems calling Dispose inside of the anonymous method below? Recommendations of a better approach?

Are there any downsides to doing the following? Bad idea?

delayTimer = new Timer() { Interval = 2000 };
{
    delayTimer.Tick += (sender2, e2) => 
    { 
        ((Timer)sender2).Stop();     
        MessageBox.Show("Do something after 2000ms"); 
        delayTimer.Dispose(); 
    };
}

Upvotes: 0

Views: 1904

Answers (4)

Enigmativity
Enigmativity

Reputation: 117074

I didn't have any serious performance issues creating a thousand timers with your code.

Disposing of a Timer (or any IDisposable) within an anonymous method is no problem at all.

However, your code isn't written in the best way possible. Try this implementation instead:

    var delayTimer = new Timer()
    {
        Interval = 2000,
        Enabled = true,
    };
    EventHandler tick = null;
    tick = (_s, _e) => 
    { 
        delayTimer.Tick -= tick;
        delayTimer.Stop();     
        delayTimer.Dispose(); 
        MessageBox.Show("Do something after 2000ms");
    };
    delayTimer.Tick += tick;

It's always a good idea to detach the event before trying to dispose of the timer. In fact there may be many times that failing to detach will not allow the GC to clean up properly and you could have a memory leak.

Nevertheless I do like the Rx answer to this question as the cleanest way to go. Although using Rx doesn't marshall the callback on the UI thread unless you do this:

Observable
    .Timer(TimeSpan.FromSeconds(2.0))
    .ObserveOn(this) // assuming `this` is your form
    .Subscribe(_ => MessageBox.Show("Do something after 2000ms"));

Much simpler.

Upvotes: 0

Dan Bryant
Dan Bryant

Reputation: 27505

One thing you can do differently is move the Dispose for the timer to the front of the anonymous method. This way, even if you throw an exception later in the method, you've still Disposed the timer. I've used this kind of pattern before and it's a reasonably clean way to get a delayed callback.

If you're using C#5, there is a very nice Task.Delay method you can await to get a timer callback within an async method. This is often used to implement timeouts in combination with a call to WaitAny like this:

    public static async Task WithTimeout(this Task task, int timeout, string timeoutMessage = null, params object[] args)
    {
        var timeoutTask = Task.Delay(timeout);
        if (await Task.WhenAny(task, timeoutTask) == timeoutTask)
            throw new TimeoutException(timeoutMessage == null ? "Operation timed out" : string.Format(timeoutMessage, args));

        await task;
    }

Upvotes: 1

Sergii Vashchyshchuk
Sergii Vashchyshchuk

Reputation: 970

You need a cache only in case if you create timers really very often (hundreds times per second), because multiple creation and disposing such heavy object as Timer (it uses some system resources) can lead to a performance issue. But from your detailed description it is obvious that you are not going to create timers so often, so you can keep your solution. But instead of using timer, you can use RX's Interval observable collection, and all your code will be shortened to 1 string:

Observable.Interval(TimeSpan.FromSeconds(2)).Take(1).Subscribe(_ => MessageBox.Show("Do something after 2000ms"));

Upvotes: 1

Sergii Vashchyshchuk
Sergii Vashchyshchuk

Reputation: 970

Very strange task, but ok.

If you really need to do this, you really want to use a timer, and that block of code will really be called very often, than you should consider to use a cache of timers.

Each time the block of code is called, you should check the cache whether it contains a free timer. If yes, than just use the first available, otherwise, create a new one and put it into the cache. Also, you will need an another timer which will work all the time and will regularly check the cache for an unused timers. If some timer or timers are unused for lets say 10 seconds, than dispose it and remove from cache. Such approach will significantly reduce the number of created instances of timers.

Upvotes: 1

Related Questions