System.Threading.Timer keep reference to it

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.Threadi开发者_运维技巧ng.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.

---

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

---

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

---

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.

---

The code "working" is indeed a side-effect of a non-deterministic Garbage Collection / finalizers.

This code, running in LINQ Pad as C# Statements, shows the issue - no messages will be logged because the Timer is GC'ed (and the finalizer is called and it cleans up the internal timer resources..)

new System.Threading.Timer((o) => { "Hi".Dump(); }, this, 100, 100);
GC.Collect();
Thread.Sleep(2000);

However, comment out the "GC.Collect" statement and messages will be logged for 2 seconds as Garbage Collection is not [immediately] performed the Timer's finalizer is not called prior to the program ending.

Since the behavior is non-deterministic, it should also be considered a bug to rely on :}

The same issue exists in the follow on code because a strong reference is required to ensure an object is not GC'ed - in that example, there is still no reference kept to the timer wrapper object so the same issue exists, albeit with one more level of indirection..