Sequential execution

Question

I made a class that I think satisfies the conditions:

1. Actions can be added to the queue at any time by any number of clients.

2. Action's executing in the order in which were added.

3. Only one Action executing at time.

public sealed class SequentialTaskScheduler
{
 private static volatile SequentialTaskScheduler instance = null;
 private static readonly object padlock = new object();
 private readonly ConcurrentQueue<Action> queue = new ConcurrentQueue<Action>();
 SequentialTaskScheduler()
 {
 var task = Task.Factory.StartNew(ThreadProc);
 }
 public static SequentialTaskScheduler Instance
 {
 get
 {
 if (instance == null)
 {
 lock (padlock)
 {
 if (instance == null)
 {
 instance = new SequentialTaskScheduler();
 }
 }
 }
 return instance;
 }
 }
 public void AddToQueueForExecution(Action action)
 {
 queue.Enqueue(action);
 }
 private void ThreadProc()
 {
 while (true)
 {
 Action item;
 bool isSuccessfull = false;
 isSuccessfull = queue.TryDequeue(out item);
 if (isSuccessfull)
 {
 item();
 }
 System.Threading.Thread.Sleep(100);
 }
 }
}

I will be grateful for the advice about dangers of such an implementation. And also will be grateful for the advice how to improve it.

Update:

I get feedback on this task from company.

• uncertain use of volatile

• use threadsafe singleton or singleton

• Task.Factory.StartNew take pool from thread but does not return it.

• use busy loop with Thread.Sleep is not elegant solution.

How to improve code with those remark's? I think that first two reason are non constructive, but the third and four are constructive. i will be grateful for the advices.

• Jesse C. Slicer: Required reading: csharpindepth.com/Articles/General/Singleton.aspx
• paparazzo: You are aware of BlockingCollection? msdn.microsoft.com/en-us/library/dd267312(v=vs.110).aspx
• Ted: @paparazzo, it was test task. With 3 condtions. Implementation could be whatever.

Answer 1

I have few suggestions, and modified the code.

1. Use Lazy singleton pattern instead of Double check singleton, avoid usage of volatile

   private static readonly Lazy<SequentialTaskExecutor> _lazy = new 
   Lazy<SequentialTaskExecutor>(() => new SequentialTaskExecutor());
   

2. Tasks should be cancelled and disposed properly. Since the task we want to run for long time, create a new task using Task.Factory.StartNew with Cancellation tokens and TaskCreationOptions.LongRunning

   Task.Factory.StartNew(this.ExecuteTask, _token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
   

3. Class should implement IDisposable interface, so created threads can be cancelled properly and items in the queue are cleared

   public class SequentialTaskExecutor : IDisposable
   protected virtual void Dispose(bool disposing)
   {
    try
    {
    if (!disposedValue)
    {
    if (disposing)
    {
    // clear the items in queue.
    Action item;
    while (_queue.Count != 0)
    {
    if (_queue.TryDequeue(out item))
    {
    }
    }
    _tokenSource.Cancel();
    _executionTask.Wait();
    _tokenSource.Dispose();
    _executionTask.Dispose();
    }
    disposedValue = true;
    }
    }
    catch
    {
    }
    finally
    {
    _tokenSource = null;
    _executionTask = null;
    }
   }
   

4. Use Smart waiting instead of Busy waiting in tasks.

   private void ExecuteTask()
   {
    bool isTaskCompleted = true;
    while (true)
    {
    Action item;
    if (_token.IsCancellationRequested)
    {
    while (_queue.Count != 0)
    {
    if (_queue.TryDequeue(out item))
    {
    }
    }
    break;
    }
    if (isTaskCompleted)
    {
    isTaskCompleted = false;
    var innerTask = Task.Factory.StartNew(() =>
    {
    if(_token.IsCancellationRequested)
    {
    return;
    }
    if(_queue.TryDequeue(out item))
    {
    item();
    }
    }, _token).ContinueWith((ants) =>
    {
    if(ants.IsCanceled || ants.IsFaulted)
    {
    isTaskCompleted = true;
    }
    else if(ants.IsCompleted)
    {
    isTaskCompleted = true;
    }
    _currentInterval = _minimumInterval;
    System.Diagnostics.Debug.WriteLine(Task.CurrentId + " " + DateTime.Now.ToString() + " Task execution complted _currentInterval : " + _currentInterval);
    });
    }
    else
    {
    // Raise the interval till maximum value
    if (_currentInterval < _maximumInterval)
    {
    _currentInterval++;
    }
    System.Diagnostics.Debug.WriteLine(Task.CurrentId + " " + DateTime.Now.ToString() + " Task waiting complted _currentInterval : " + _currentInterval);
    Thread.Sleep(TimeSpan.FromSeconds(_currentInterval));
    }
    }
   }
   

5. Complete source code is as follows

   public class SequentialTaskExecutor : IDisposable
   {
    private static readonly Lazy<SequentialTaskExecutor> _lazy = new Lazy<SequentialTaskExecutor>(() => new SequentialTaskExecutor());
    private readonly ConcurrentQueue<Action> _queue = null;
    private CancellationTokenSource _tokenSource = null;
    private CancellationToken _token;
    private int _maximumInterval = 10;
    private int _minimumInterval = 1;
    private int _currentInterval = 0;
    private bool disposedValue = false;
    Task _executionTask = null;
    private SequentialTaskExecutor()
    {
    _queue = new ConcurrentQueue<Action>();
    _tokenSource = new CancellationTokenSource();
    _token = _tokenSource.Token;
    _executionTask = Task.Factory.StartNew(this.ExecuteTask, _token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
    }
    public static SequentialTaskExecutor Instance
    {
    get
    {
    return _lazy.Value;
    }
    }
    public void AddToQueueForExecution(Action action)
    {
    _queue.Enqueue(action);
    System.Diagnostics.Debug.WriteLine("Task added " + _queue.Count);
    }
    private void ExecuteTask()
    {
    bool isTaskCompleted = true;
    while (true)
    {
    Action item;
    if (_token.IsCancellationRequested)
    {
    while (_queue.Count != 0)
    {
    if (_queue.TryDequeue(out item))
    {
    }
    }
    break;
    }
    if (isTaskCompleted)
    {
    isTaskCompleted = false;
    var innerTask = Task.Factory.StartNew(() =>
    {
    if(_token.IsCancellationRequested)
    {
    return;
    }
    if(_queue.TryDequeue(out item))
    {
    item();
    }
    }, _token).ContinueWith((ants) =>
    {
    if(ants.IsCanceled || ants.IsFaulted)
    {
    isTaskCompleted = true;
    }
    else if(ants.IsCompleted)
    {
    isTaskCompleted = true;
    }
    _currentInterval = _minimumInterval;
    System.Diagnostics.Debug.WriteLine(Task.CurrentId + " " + DateTime.Now.ToString() + " Task execution complted _currentInterval : " + _currentInterval);
    });
    }
    else
    {
    // Raise the interval till maximum value
    if (_currentInterval < _maximumInterval)
    {
    _currentInterval++;
    } 
    System.Diagnostics.Debug.WriteLine(Task.CurrentId + " " + DateTime.Now.ToString() + " Task waiting complted _currentInterval : " + _currentInterval);
    Thread.Sleep(TimeSpan.FromSeconds(_currentInterval));
    }
    }
    }
    protected virtual void Dispose(bool disposing)
    {
    try
    {
    if (!disposedValue)
    {
    if (disposing)
    {
    // clear the items in queue.
    Action item;
    while (_queue.Count != 0)
    {
    if (_queue.TryDequeue(out item))
    {
    }
    }
    _tokenSource.Cancel();
    _executionTask.Wait();
    _tokenSource.Dispose();
    _executionTask.Dispose();
    }
    disposedValue = true;
    }
    }
    catch
    {
    }
    finally
    {
    _queue = null;
    _tokenSource = null;
    _executionTask = null;
    }
    }
    public void Dispose()
    {
    Dispose(true);
    }
    }
   }
   

Answer 2

I mostly agree with vishnu vardhan's answer. However I disagree with his 4th suggestion, which I think is hardly an improvement.

It still uses Thread.Sleep which should never be used in production code: you shouldn't pay your worker to sleep. Either use a BlockingCollection or write your own synchronization using ManualResetEvent. If no tasks are being processes your thread should just hang on some wait handle. It should not spin in a loop, constantly consuming CPU resources, while doing no meaningful work.

I also think it might be a good idea to refactor your code into an actual TaskScheduler.

P.S. Common pitfall, when using Task.Factory.