11
\$\begingroup\$

A user must query a large database for information repeatedly. This query may take up to ~7 seconds. This must be done asynchronously. Only one query will be running at a time.

I did not create a new thread for each query as the code would be less clean, less maintable, etc.

I did not use ThreadPool.QueueUserWorkItem as this will tie up a ThreadPool thread. I'd rather not increase the minimum ThreadPool threads for this via ThreadPool.SetMinThreads as this won't be used by every user.

Considering the above, and that this problem seems to come up in this project often, I decided to create my own single thread "ThreadPool" which just processes items. No new threads created, no ThreadPool thread occupied, reusable elsewhere...

Should I have avoided the built-in .net ThreadPool? Is there a better approach I've missed?

I've included source for clarity. I'm not looking for code critique as much as I am advice on what to do in this scenario. I do not have access to async/await, for those who want to critique source.

SingleThreadPool

public class SingleThreadPool : IThreadPool
{
    private readonly Thread thread;
    private readonly Queue<CallbackTask> queue;
    private readonly AutoResetEvent resetEvent;

    public SingleThreadPool()
    {
        this.queue = new Queue<CallbackTask>();
        this.resetEvent = new AutoResetEvent(false);
        this.IsDisposed = false;
        this.thread = new Thread(Run);
        this.thread.IsBackground = true;
        this.thread.Name = "SingleThreadPool";
        this.thread.Start();
    }

    ~SingleThreadPool()
    {
        this.Dispose(true);
    }

    public void Dispose()
    {
        this.Dispose(false);
        //keeps destructor from executing.
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool calledFromFinalizer)
    {
        if (!this.IsDisposed)
        {
            CallbackTask[] cpy = null;
            lock (this.queue)
            {
                cpy = this.queue.ToArray();
                queue.Clear();
            }
            //do not call cancel within locked block.
            foreach (var item in cpy)
            {
                item.TaskItem.Cancel();
            }
            this.IsDisposed = true;
            this.resetEvent.Set();
            //DO NOT JOIN IF CALLED FROM FINALIZER! CAN CAUSE DEADLOCK!
            //the GC may put other threads to sleep while calling finalizer.
            //if it puts this.thread to sleep, we may deadlock.
            if (!calledFromFinalizer)
            {
                this.thread.Join();
            }
        }
    }

    public void QueueUserWorkItem(ITaskItem item,
        Action<TaskResult> taskFinishedCallback)
    {
        if (item == null)
            throw new ArgumentNullException("Item cannot be null");
        lock (this.queue)
        {
            CallbackTask callbackTask = new CallbackTask(item, taskFinishedCallback);
            this.queue.Enqueue(callbackTask);
        }
        this.resetEvent.Set();
    }

    protected bool IsDisposed { get; private set; }

    private void Run()
    {
        while (!this.IsDisposed)
        {
            CallbackTask item = null;
            lock (this.queue)
            {
                if (this.queue.Count > 0)
                {
                    item = this.queue.Dequeue();
                }
            }
            if (item != null)
            {
                this.ExecuteItem(item);
            }
            this.resetEvent.WaitOne();
        }
    }

    private void ExecuteItem(CallbackTask item)
    {
        try
        {
            item.TaskItem.AsyncProcessTask();
            item.FireFinishedCallback(null);
        }
        catch (Exception e)
        {
            string msg = "Exception executing task " + item + ", Exception: " + e;
            Console.WriteLine(msg);
            item.FireFinishedCallback(e);
        }
    }

    private class CallbackTask
    {
        public ITaskItem TaskItem { get; private set; }
        private readonly Action<TaskResult> finishedCallback;

        public CallbackTask(ITaskItem item, Action<TaskResult> finishedCallback)
        {
            if (item == null)
                throw new ArgumentNullException("Item cannot be null");
            this.TaskItem = item;
            this.finishedCallback = finishedCallback;
        }

        public void FireFinishedCallback(Exception e)
        {
            if (this.finishedCallback != null)
            {
                this.finishedCallback(new TaskResult(this.TaskItem, e));
            }
        }
    }
}

ITaskItem

public interface ITaskItem
    {
        /// <summary>
        /// Item should end execution if in process.
        /// </summary>
        void Cancel();
        /// <summary>
        /// Perform some task, if not cancelled.
        /// </summary>
        void AsyncProcessTask();
    }

TaskResult

public class TaskResult
    {
        private readonly Exception exception;
        private readonly ITaskItem item;

        public TaskResult(ITaskItem item, Exception e)
        {
            this.item = item;
            this.exception = e;
        }

        public ITaskItem Finish()
        {
            if (this.exception != null)
            {
                throw this.exception;
            }
            return this.item;
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ What advice are you seeking? You are either wasting a thread if these requests are infrequent or slowing users making these if frequent. Neither of these will be terrible, but you could instead build an executor service that would use up to N threads from the main pool at a time. This minimizes unused threads and improves performance when several users perform these queries at once. \$\endgroup\$ – David Harkness Feb 14 '14 at 3:44
  • \$\begingroup\$ You suggest using the threadpool, but limiting the number of concurrent queries correct? Not good, as one or more threadpool threads will be tied up for ~7 seconds. If I tie them all up, no one else can use the threadpool for that time period. \$\endgroup\$ – William Morrison Feb 14 '14 at 4:21
  • \$\begingroup\$ The point of a thread pool is to provide concurrent access without overloading the CPUs. Allocating up to four threads would only block up to four clients, but they would only for the next quick call to complete. Extra service calls for this (slow) method would sit in their own segregated queue while other calls would sit in the main pool's queue. \$\endgroup\$ – David Harkness Feb 14 '14 at 4:27
  • \$\begingroup\$ Please post an answer. I can't tell if you're saying this is a built in feature of .net's threadpool, or your suggesting some class I've never heard of or that I build my own or what... \$\endgroup\$ – William Morrison Feb 14 '14 at 4:37
  • \$\begingroup\$ I do not know C#, but that it has a thread pool and queue tells me this is possible. I will post some psuedocode tomorrow when I'm not on my phone. \$\endgroup\$ – David Harkness Feb 14 '14 at 4:46
2
\$\begingroup\$

I did not use ThreadPool.QueueUserWorkItem as this will tie up a ThreadPool thread.

But that's pretty much what the ThreadPool is for: for you to submit your jobs to it and tie it up for some time. The ThreadPool is smart, so if it detects that some of its threads are blocking, it will create new threads faster. This means that your submitting of this job to the ThreadPool shouldn't block other code that also uses it (or at least not for long).

But there are also some ways to do this than just QueueUserWorkItem():

  1. Use the Task type from .Net 4.0, specifically Task.Run() if you're using .Net 4.5 or Task.Factory.StartNew() on .Net 4.0.
  2. Make your code truly asynchronous (which won't tie up a thread at all). This assumes your database code supports it. But, this is probably not worth it if you can't use async-await. (Note: you can use it even if you target .Net 4.0, but it requires VS 2012 and Microsoft.Bcl.Async.)
\$\endgroup\$
  • \$\begingroup\$ Thanks, I didnt realize the threadpool allowed so many maximum active threads. Its silly to worry about tying one up. \$\endgroup\$ – William Morrison Feb 23 '14 at 16:58
2
\$\begingroup\$

If you are using .Net 4.0 you can use Tasks:

       // Create a scheduler that uses two threads. 
       LimitedConcurrencyLevelTaskScheduler lcts = 
         new LimitedConcurrencyLevelTaskScheduler(1);
       List<Task> tasks = new List<Task>();

       // Create a TaskFactory and pass it our custom scheduler. 
       TaskFactory factory = new TaskFactory(lcts);

       //...

      public void InitiateNewTask() {
          Task t = factory.StartNew(() => {
              // your code here
              }, cts.Token);
          tasks.Add(t);                      
      }
\$\endgroup\$
1
\$\begingroup\$

As it stands, I do not really see any problem. You are using the thread as I would think it should be used.

One note, you could add a priority to the queue so someone could be added at the top of the queue instead of the bottom. That way that someone is next in line and won't have to wait for all the other queries to end before he can run his own (that's good for the boss... but seriously, if you have say 5 people who use that feature all the time, they should probably have higher priority than the 20 others who can wait 30 seconds once for their one query a day.)

The one other possibility, although you seem to already have had it tested and not viewed as a good solution, is to have 2 threads running in parallel. When compiling, using -J 4 or -J 8 will improve the compilation process because it can make use of multiple processors and while compiler A reads a file, compiler B can compile what it read earlier. If that applies to your database, one thread does some work with the CPU, then another thread can access the database by itself.

You could also go further where you allow many threads, but any query to the database in regard to these jobs are serialized (you use a lock() and only one of the threads can access the database at a time). This is quite useful if you have much work to do with the data read before writing it back to the database. It is useless if the SQL database does all the work itself.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.