Compare using Thread.Sleep and Timer for delayed execution

I have a method which should be delayed running for a specified amount of time.

Should I use

Thread thread = new Thread(() => {
    Thread.Sleep(millisecond);
    action();
});
thread.IsBackground = true;
thread.Start();

Or

Timer timer = new Timer(o => action(), null, millisecond, -1);

I had read some articles about using Thread.Sleep is bad design. But I don't really understand why.

But for using Timer, Timer has dispose method. Since the execution is delayed, I don't know how to dispose Timer. Do you have any suggestions?

Or if you have alternative codes for delayed execution are also appreciate.

One difference is that System.Threading.Timer dispatches the callback on a thread pool thread, rather than creating a new thread every time. If you need this to happen more than once during the life of your application, this will save the overhead of creating and destroying a bunch of threads (a process which is very resource intensive, as the article you reference points out), since it will just reuse threads in the pool, and if you will have more than one timer going at once it means you will have fewer threads running at once (also saving considerable resources).

In other words, Timer is going to be much more efficient. It also may be more accurate, since Thread.Sleep is only guaranteed to wait at LEAST as long as the amount of time you specify (the OS may put it to sleep for much longer). Granted, Timer is still not going to be exactly accurate, but the intent is to fire the callback as close to the specified time as possible, whereas this is NOT necessarily the intent of Thread.Sleep.

As for destroying the Timer, the callback can accept a parameter, so you may be able to pass the Timer itself as the parameter and call Dispose in the callback (though I haven't tried this -- I guess it is possible that the Timer might be locked during the callback).

Edit: No, I guess you can't do this, since you have to specify the callback parameter in the Timer constructor itself.

Maybe something like this? (Again, haven't actually tried it)

class TimerState
{
    public Timer Timer;
}

...and to start the timer:

TimerState state = new TimerState();

lock (state)
{
    state.Timer = new Timer((callbackState) => {
        action();
        lock (callbackState) { callbackState.Timer.Dispose(); }
        }, state, millisecond, -1);
}

The locking should prevent the timer callback from trying to free the timer prior to the Timer field having been set.

---

Addendum: As the commenter pointed out, if action() does something with the UI, then using a System.Windows.Forms.Timer is probably a better bet, since it will run the callback on the UI thread. However, if this is not the case, and it's down to Thread.Sleep vs. Threading.Timer, Threading.Timer is the way to go.

use ThreadPool.RegisterWaitForSingleObject instead of timer:

//Wait 5 seconds then print out to console. 
//You can replace AutoResetEvent with a Semaphore or EventWaitHandle if you want to execute the command on those events and/or the timeout
System.Threading.ThreadPool.RegisterWaitForSingleObject(new AutoResetEvent(false), (state, bTimeout) => Console.WriteLine(state), "This is my state variable", TimeSpan.FromSeconds(5), true);

I think Thread.Sleep is fine if you really want to pause the application for a specified amount of time. I think the reason people say it is a bad design is because in most situations people don't actually want the application to pause.

For example, I was working on a pop3 client where the programmer was using Thread.Sleep(1000) to wait while the socket retrieved mail. In that situation it was better to hook up an event handler to the socket and continuing program execution after the socket had completed.

I remember implementing a solution similar to Eric's one. This is however a working one ;)

class OneTimer
    {
        // Created by Roy Feintuch 2009
        // Basically we wrap a timer object in order to send itself as a context in order to dispose it after the cb invocation finished. This solves the problem of timer being GCed because going out of context
        public static void DoOneTime(ThreadStart cb, TimeSpan dueTime)
        {
            var td = new TimerDisposer();
            var timer = new Timer(myTdToKill =>
            {
                try
                {
                    cb();
                }
                catch (Exception ex)
                {
                    Trace.WriteLine(string.Format("[DoOneTime] Error occured while invoking delegate. {0}", ex), "[OneTimer]");
                }
                finally
                {
                    ((TimerDisposer)myTdToKill).InternalTimer.Dispose();
                }
            },
                        td, dueTime, TimeSpan.FromMilliseconds(-1));

            td.InternalTimer = timer;
        }
    }

    class TimerDisposer
    {
        public Timer InternalTimer { get; set; }
    }

The only beef that I have with the System.Timer is that most of the time I have seen it used for long delays (hours, minutes) in polling services and developers often forget to launch the event Before they start the timer. This means that if I start the app or service, I have to wait until the timer elapses (hours, minutes) before it actually executes.

Sure, this is not a problem with the timer, but I think that its often used improperly by because its just too easy to misuse.

@miniscalope No don't use ThreadPool.RegisterWaitForSingleObject instead of timer, System.Threading.Timer will queue a callback to be executed on a thread pool thread when the time has elapsed and doesn't require a wait handle, wait for single object will tie up a threadpool thread waiting for the event to be signalled or the timeout to expire before the thread calls the callback.