Producer/Consumer queue

Question

I use this class for scheduling bits of work on a single thread. Work can be scheduled to be executed immediately or at a future time with the use of timers. It has many producers (the work adders) and a single consumer (the thread which executes the work).

This class works well but I'm under no illusion that it's perfect. I'm looking for potential problems this class may exhibit especially relating to threading (dead locks etc).

internal delegate void MainLoopTask();
internal delegate object MainLoopJob();

internal static class MainLoop
{
    private class DelegateTask
    {
        private bool _isBlocking;
        private Exception _storedException;
        private MainLoopTask _task;
        private MainLoopJob _job;
        private object _jobResult;
        private ManualResetEvent _handle;

        public DelegateTask()
        {

        }

        public bool IsBlocking
        {
            get { return _isBlocking; }
            set { _isBlocking = value; }
        }

        public Exception StoredException
        {
            get { return _storedException; }
            set { _storedException = value; }
        }

        public object JobResult
        {
            get { return _jobResult; }
        }

        public MainLoopTask Task
        {
            get { return _task; }
            set { _task = value; }
        }

        public MainLoopJob Job
        {
            get { return _job; }
            set { _job = value; }
        }

        public ManualResetEvent WaitHandle
        {
            get 
            { 
                if (_handle == null)
                    _handle = new ManualResetEvent(false);
                return _handle; 
            }
        }

        public void Execute()
        {
            try
            {
                if (_task != null)
                    _task();
                if (_job != null)
                    _jobResult = _job();
            }
            catch (Exception ex)
            {
                _storedException = ex;
                if (!_isBlocking)
                    throw;
            }
            finally
            {
                if (_isBlocking)
                    _handle.Set();
            }
        }
    }

    private class TaskInfo
    {
        public Timer timer;
        public DelegateTask task;
    }

    private static AutoResetEvent _handle = new AutoResetEvent(false);
    private static Queue<DelegateTask> _tasks = new Queue<DelegateTask>();
    private static List<Timer> _timers = new List<Timer>();
    private static volatile bool _running;
    private static Thread _thread;

    public static void Loop()
    {
        _thread = Thread.CurrentThread;
        _running = true;
        while (_running)
        {
            DelegateTask task = null;

            lock (_tasks)
            {
                if (_tasks.Count > 0)
                    task = _tasks.Dequeue();
            }

            if (task == null)
            {
                _handle.WaitOne();
            }
            else
            {
                task.Execute();
            }
        }
    }

    public static void Quit()
    {
        _running = false;
        _handle.Set();
    }

    private static void Queue(DelegateTask task)
    {
        lock (_tasks)
        {
            _tasks.Enqueue(task);
            _handle.Set();
        }
    }

    public static void Queue(MainLoopTask task)
    {
        DelegateTask dTask = new DelegateTask();
        dTask.Task = task;
        Queue(dTask);
    }

    public static void QueueWait(MainLoopTask task)
    {
        DelegateTask dTask = new DelegateTask();
        dTask.Task = task;
        QueueWait(dTask);
    }

    public static object QueueWait(MainLoopJob task)
    {
        DelegateTask dTask = new DelegateTask();
        dTask.Job = task;
        QueueWait(dTask);
        return dTask.JobResult;
    }

    private static void QueueWait(DelegateTask t)
    {
        t.WaitHandle.Reset();
        t.IsBlocking = true;
        if (_thread == null || Thread.CurrentThread == _thread)
            t.Execute();
        else
            Queue(t);

        t.WaitHandle.WaitOne();
        t.WaitHandle.Close();

        if (t.StoredException != null)
        {
            throw t.StoredException;
        }
    }

    public static void QueueTimeout(MainLoopTask task, TimeSpan timeSpan)
    {
        QueueTimeout(task, (uint)timeSpan.TotalMilliseconds);
    }

    public static void QueueTimeout(MainLoopTask task, uint milliSeconds)
    {
        if (milliSeconds <= 0)
            Queue(task);
        else
        {
            DelegateTask dTask = new DelegateTask();
            dTask.Task = task;
            TaskInfo ti = new TaskInfo();
            ti.task = dTask;
            ti.timer = new Timer(WaitProc, ti, milliSeconds, Timeout.Infinite);
            lock (_timers)
                _timers.Add(ti.timer);
        }
    }

    private static void WaitProc(object state)
    {
        TaskInfo ti = (TaskInfo)state;
        lock (_timers)
            _timers.Remove(ti.timer);
        ti.timer.Dispose();
        Queue(ti.task);
    }
}

Answer 1

1. A slightly better way to wait for the queue to be not empty in your Loop method would be to use a Monitor instead of an AutoResetEvent.

   • You would get rid of _handle
   • Replace all occurrences of _handle.Set() with Monitor.Pulse(_tasks)

   • Your waiting code would look like this:

         DelegateTask task;
         lock (_tasks)
         {
             while (_tasks.Count == 0)
             {
                 Monitor.Wait(_tasks);
             }
             task = _tasks.Dequeue();
         }
     
         task.Execute();
     

   Slightly more compact code.

2. Your should get into the habit of creating dedicated objects for locking instead of locking on some data structure you happen to use. In your case it won't make much of a difference but you can get into deadlocks this way if you are not careful.

Answer 2

I don't really like the nested classes, it makes the code hard to understand since, reading it quickly, I have a hard time telling which code is in which class. Also, in C# you have auto properties, meaning you can replace

private bool _isBlocking;
public bool IsBlocking
{
    get { return _isBlocking; }
    set { _isBlocking = value; }
}

By :

public bool IsBlocking{get;set;}

Which is much cleaner and it takes less space.

Also, you have a public empty constructor, but you don't need to define it. Since you don't have any constructor overloads, you can ommit writing the default empty constructor, it will be there by... default!