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