System.Threading.Timer keep reference to it.

Question

According to [http://msdn.microsoft.com/en-us/library/system.threading.timer.aspx][1] you need to keep a reference to a System.Threading.Timer to prevent it from being disposed.

I've got a method like this:

private void Delay(Action action, Int32 ms)
    {
        if (ms <= 0)
        {
            action();
        }

        System.Threading.Timer timer = new System.Threading.Timer(
            (o) => action(), 
            null, 
            ms, 
            System.Threading.Timeout.Infinite);
    }

Which I don't think keeps a reference to the timer, I've not seen any problems so far, but that's probably because the delay periods used have been pretty small.

Is the code above wrong? And if it is, how to I keep a reference to the Timer? I'm thinking something like this might work:

    class timerstate 
    {
        internal volatile System.Threading.Timer Timer;
    };

    private void Delay2(Action action, Int32 ms)
    {
        if (ms <= 0)
        {
            action();
        }


        timerstate state = new timerstate();
        lock (state)
        {
            state.Timer = new System.Threading.Timer(
                (o) => 
                { 
                    lock (o) 
                    { 
                        action();
                        ((timerstate)o).Timer.Dispose();
                    } 
                },
                state,
                ms,
                System.Threading.Timeout.Infinite);
        }

The locking business is so I can get the timer into the timerstate class before the delegate gets invoked. It all looks a little clunky to me. Perhaps I should regard the chance of the timer firing before it's finished constructing and assigned to the property in the timerstace instance as negligible and leave the locking out.

Answer 1

Update

Thinking about your problem a bit more generally, I think what you're actually trying to accomplish here is achievable in a much simpler way, without using a System.Threading.Timer at all.

Is this basically what you want your method to do? Perform action after a specified number of milliseconds? If so, I would suggest something like the following alternative implementation instead:

private void Delay(Action action, int ms)
{
    if (ms <= 0)
    {
        action();
        return;
    }

    System.Threading.WaitCallback delayed = state =>
    {
        System.Threading.Thread.Sleep(ms);
        action();
    };

    System.Threading.ThreadPool.QueueUserWorkItem(delayed);
}

...by the way, are you aware that in the code you posted, specifying a non-zero value for ms will cause action to be executed twice?

---

Original Answer

The timerstate class really isn't necessary. Just add a System.Threading.Timer member to whatever class contains your Delay method; then your code should look like this:

public class Delayer
{
    private System.Threading.Timer _timer;

    private void Delay(Action action, Int32 ms)
    {
        if (ms <= 0)
        {
            action();
        }

        _timer = new System.Threading.Timer(
            (o) => action(), 
            null, 
            ms, 
            System.Threading.Timeout.Infinite);
    }
}

Now, I see that you are specifying the period argument of the timer's constructor as System.Threading.Timeout.Infinite (-1). What this means is that you intend for your timer to call action once, after ms has elapsed; am I right? If this is the case, then there's actually not much need to worry about the timer being disposed anyway (i.e., it will be, and that's fine), assuming a relatively low value for ms.

Anyway, if you're going to hold onto an instance of an IDisposable object (like System.Threading.Timer), you should generally dispose of that member when your object (i.e., this instance) is disposed of. I believe System.Threading.Timer has a finalizer that will cause it to be disposed of eventually anyway, but it's best to dispose of things as soon as you don't need them anymore. So:

public class Delayer : IDisposable
{
    // same code as above, plus...

    public void Dispose()
    {
        _timer.Dispose();
    }
}

Answer 2

Your second approach wouldn't keep the reference either. After the end of the Delay2-block, the reference to state is gone so the Garbage Collector will collect it ... then your reference to Timer is gone, too and it will be collected and disposed.

class MyClass
{
    private System.Threading.Timer timer;

    private void Delay(Action action, Int32 ms)   
    {   
        if (ms <= 0)   
        {   
            action();   
        }   

        timer = new System.Threading.Timer(   
            (o) => action(),    
            null,    
            ms,    
            System.Threading.Timeout.Infinite);   
    }   
}

Answer 3

I read from your comments to the existing answers that you can have 0..n Actions and so you would have 0..n Timers, too. Is that right? In this case you should do one of the following:

1. Keep a List/Dictionary of timers, but in this case you have to remove the timer after firing.
2. Build a scheduler: have 1 Timer, that fired regulary, for each Delay-call add the action an the calculated time when it should run into a List/Dictionary, each time the timer fired, check you list and run&remove the Action. You can even build this scheduler that it sorts the Actions by execution time an sets the Timer to an adequate interval.