views:

4681

answers:

7

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.

Thank you,

+4  A: 

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.

Shawn Simon
Hitting the nail on the head.
StingyJack
Except in neither example in the post is the application actually pausing. The poster is doing the pause on a separate thread, not calling Thread.Sleep directly. Calling Thread.Sleep directly, yes, bad idea.
Eric Rosenberger
+6  A: 

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.

Eric Rosenberger
It's also worth pointing out the difference with System.Windows.Forms.Timer, which I *believe* calls a function on the UI thread, which is really important for WinForms apps!
Dave Markle
A: 

As above answer said, creating new thread is resource intensive. Should I use ThreadPool instead?

ThreadPool.QueueUserWorkItem(o => {
    Thread.Sleep(millisecond);
    action();
});
chaowman
This is not a good idea, because it ties up one of the threads in the thread pool waiting for the event. Like I said in my post, using System.Threading.Timer is more efficient than trying to use Sleep.
Eric Rosenberger
+1'd because the -1 on a newbie for answering instead commenting was entirely unnecessary.
Daniel Schaffer
Addendum to comment: Timer calls the callback method on a thread pool thread; it doesn't put a separate thread pool thread to sleep for every individual timer waiting for the event as in this answer. So if you have multiple timers waiting at once, it does NOT tie up multiple thread pool threads.
Eric Rosenberger
A: 

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.

StingyJack
+1  A: 

use ThreadPool.RegisterWaitForSingleObject instead of timer

Read the accepted answer. Timer already uses the thread pool.
John Saunders
I prefer RegisterWaitForSingleObject for the logic...The method is executed ONCE when the time is up... so you dont have to trick to stop the timer once the job is done that is not good...so RegisterWaitForSingleObject naturaly do exactly what he want, timer dont timer is better when you want execute a task several times at specific intervals....
A: 

@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.

Matt
A: 

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