Running Legacy, Non-Reentrant Code on a Background Thread in .NET

Question

I'm in needed of a thread worker in my .NET application - .NET has several classes such as thread pools etc., but I can't find anything that runs on a single thread, which is a requirement in my case.

So I've had a go a writing one myself, but these things are notoriously tricky, I'm sure I've got something wrong. Can anyone improve it or point me in the direction of somthing similar that's already been written?

public class ThreadWorker : IDisposable
{
    public ThreadWorker()
    {
        m_Thread = new Thread(ThreadMain);
        m_Thread.IsBackground = true;
        m_Thread.Name = "Worker Thread";
        m_Thread.Start();
    }

    public void Dispose()
    {
        if (!m_Terminate)
        {
            m_Terminate = true;
            m_Event.Set();
            m_Thread.Join();
        }
    }

    public void QueueUserWorkItem(Action<object> callback, object data)
    {
        lock (m_Queue) m_Queue.Enqueue(new WorkItem(callback, data));
        m_Event.Set();
    }

    void ThreadMain()
    {
        while (!m_Terminate)
        {
            if (m_Queue.Count > 0)
            {
                WorkItem workItem;
                lock (m_Queue) workItem = m_Queue.Dequeue();
                workItem.Callback(workItem.Data);
            }
            else
            {
                m_Event.WaitOne();
            }
        }
    }

    class WorkItem
    {
        public WorkItem(Action<object> callback, object data)
        {
            Callback = callback;
            Data = data;
        }

        public Action<object> Callback;
        public object Data;
    }

    AutoResetEvent m_Event = new AutoResetEvent(false);
    Queue<WorkItem> m_Queue = new Queue<WorkItem>();
    Thread m_Thread;
    bool m_Terminate;
}

C'mon, Tear it apart!

PLEASE STOP ASKING WHETHER I NEED THIS: Yes I do - I have legacy C code that isn't thread-safe, so I need to synchronise all of my calls to it on a background thread.

EDIT: Last minute change to my code was obviously incorrect ,fixed it.

Answer 1

I think you're reinventing the wheel. The supplied System.Threading.ThreadPool provides this functionality. I would just use that and get back to writing actual application logic.

Answer 2

I'll assume that you actually have a good reason for wanting a thread pool that only has one thread. In that case, I only spot one major flaw: Why are you using Action<T> when you are always passing null? Just use Action, which takes no arguments.

Answer 3

I'm going to ignore the question of whether or not you should be doing this and just give you feedback about your code. Most are style issues or suggestions to use best practices, but others are bugs that need to be fixed.

1. Don't use Hungarian notation (m_*). It's not necessary.
2. You need to lock when accessing m_Terminate. Or at the very least it needs to be volatile.
3. Why are you using Action<object> and then passing null as a parameter? Can't you just use ThreadStart if you don't want a parameter? Fixed
4. WorkItem should be immutable. Use readonly members or a property with a private setter.
5. Missing error handling. If one of your work item actions throws an exception it will stop your entire thread pool working (assuming it doesn't take your entire application down).
6. I have to agree with Greg's comment. This isn't a thread pool. It's a work queue. The name of the class should reflect that.
7. You should validate that the parameter callback to QueueUserWorkItem is not null to avoid NullReferenceException in the loop in ThreadMain (fail fast).
8. You should throw an ObjectDisposedException if QueueUserWorkItem is called after Dispose has already been called.

Answer 4

Its in one of the comments, but the BackgroundWorker, is essentially a good implementation of what you want. Though you could start several of them, in your case just start one.

Secondarily, you actually can use ThreadPool for this, by changing your strategy:

ThreadPool.QueueUserWorkItem(myWorkerProcess);

Then:

void myWorkerProcess(object state) {
   WorkItem workItem;

   while (!m_Terminate) {
      m_Event.WaitOne(5000);
      if (m_Queue.Count > 0) {
         lock (m_Queue) workItem = m_Queue.Dequeue();
         // ... do your single threaded operation here ...
      }
   }
}

In this way you only ever have one background thread and it just loops waiting for you to do your thing.

Answer 5

Hehe, I wrote something very similar recently.

With using the AutoResetEvent for synchronization, if you queue work items faster than they are processed, you end up with items in your queue, but no event triggering the processing of the items. I suggest using a semaphore so that you can maintain a count of how many times you've been signaled, such that you process all work items.