Is it always bad to use Thread.Sleep()?

Question

I created an extension method for the the class Random which executes an Action (void delegate) at random times:

public static class RandomExtension
{
    private static bool _isAlive;
    private static Task _executer;

    public static void ExecuteRandomAsync(this Random random, int min, int max, int minDuration, Action action)
    {
        Task outerTask = Task.Factory.StartNew(() =>
        {
            _isAlive = true;
            _executer = Task.Factory.StartNew(() => { ExecuteRandom(min, max, action); });
            Thread.Sleep(minDuration);
            StopExecuter();
        });
    }

    private static void StopExecuter()
    {
        _isAlive = false;
        _executer.Wait();

        _executer.Dispose();
        _executer = null;
    }

    private static void ExecuteRandom(int min, int max, Action action)
    {
        Random random = new Random();

        while (_isAlive)
        {
            Thread.Sleep(random.Next(min, max));
            action();
        }
    }
}

It works fine.

But is the use of Thread.Sleep() in this example okay, or should you generally never use Thread.Sleep(), what complications could occur ? Are there alternatives?

Answer 1

Is using Thread.Sleep bad? Generally not, if you really want to suspend the thread. But in this case you don't want to suspend the thread, you want to suspend the task.

So in this case, you should use:

await Task.Delay(minDuration);

This will not suspend the entire thread, but just the single task you want to suspend. All other tasks on the same thread can continue running.

Answer 2

One reason I would use Task.Delay over Thread.Sleep is the fact that you can pass a CancellationToken to it. If the user wants to StopExecutor and the random received a long duration span, you'll end up blocking for a long time. On the other hand, in Task.Delay, you can cancel the operation and it will be notified of that cancellation.

I think there are other problems with the design you choose to make. The Random class isn't really fit to be a task scheduler. I would find it a bit odd to find a ExecuteRandomAsync, since it mostly doesn't execute a random, but execute some arbitrary Action every X minutes.

I would instead, go about this another way. While keeping most of the internals you've already created, but putting them in a different class.

public class ActionInvoker
{
    private readonly Action _actionToInvoke;

    public ActionInvoker(Action actionToInvoke)
    {
        _actionToInvoke = actionToInvoke;
        _cancellationTokenSource = new CancellationTokenSource();
    }

    private readonly CancellationTokenSource _cancellationTokenSource;
    private Task _executer;

    public void Start(int min, int max, int minDuration)
    {
        if (_executer != null)
        {
            return;
        }

        _executer = Task.Factory.StartNew(
                    async () => await ExecuteRandomAsync(min, max, _actionToInvoke),
                    _cancellationTokenSource.Token, TaskCreationOptions.LongRunning, 
                    TaskScheduler.Default)
                    .Unwrap();
    }

    private void Stop()
    {
        try
        {
            _cancellationTokenSource.Cancel();
        }
        catch (OperationCanceledException e)
        {
            // Log the cancellation.
        }
    }

    private async Task ExecuteRandomAsync(int min, int max, Action action)
    {
        Random random = new Random();

        while (!_cancellationTokenSource.IsCancellationRequested)
        {
            await Task.Delay(random.Next(min, max), _cancellationTokenSource.Token);
            action();
        }
    }
}

Answer 3

Sleep "talks" to the Operation System that you want to suspend the thread. It's resource-intensive operation cause your thread uses RAM anyway (although it doesn't require processing time).

With thread pool, you could use thread's resources (f.e. RAM) to process some other small tasks. To do it, the Windows lets you put a thread to sleep in a special alertable state, so it may be awakened and used temporarily.

So Task.Delay let you put threads to alertable sleep, and therefore let you use resources of these thread unitl you don't need them.

Answer 4

Think of it this way.

Sleeping a thread is dangerous because more often than not more than one tasks relies on that thread, not to mention key components of the program may do also. Never using sleep on threads isn't appropriate, you will want to use them when they are beneficial to your program.

Lots of people are taught not to sleep threads because of the unpredictable behaviour. It's like managing a team, eventually you're going to have to let some go to lunch and as long as the rest are functioning whilst the ones you chose to go to lunch are out, then you should be fine and be able to work still.

Just don't send people to lunch if there's people in the project reliant on their presence.

Answer 5

All answers are correct, and I would like to add a practical view:

When you have a remote non real-time component to test (for example, a search engine), you need to give it time to go to it's new state before you reference it again for assertions. In other words, you want to suspend your test for some time. Since the performance the test itself is irrelevant (please distinguish between the performance of the test and the performance of the component), you sometimes prefer to keep your code (of the test) as simple and straight forward as possible, and you avoid even the minimum complexity of asynchronous code. You could of course start a new test instead of waiting for your component (that's the difference between await Task.Delay() and Thread.Sleep()), but the assumption was that you are in a no hurry for that.