A custom thread-pool/queue class

I wanted a class which executes any number of tasks but only a certain amount at the same time (e.g. to download various internet content and keep the overall download speed at a good level). The class I wrote seems to work but there are probably things that can be improved.

• Do these locks make sense? Should there be other locks as well?

• I have a method and an event that is only relevant if a specific constructor is used. Is there a way to improve that?

• The class I use for tasks is ThreadStart. Is that a good idea?

• There might be better method names/class names.

• Are there any general errors (e.g. that more Threads than the max-count will be executed)?

There are probably more points that could be improved. Also, if anyone knows a good open source library (or native .NET class even) which does just what my class is supposed to do, I would be interested in that too.

```
public class ThreadQueue
{
private readonly HashSet WorkingThreads = new HashSet();
private readonly Queue Queue = new Queue();

private bool RaiseCompleteEventIfQueueEmpty = false;

private int ThreadsMaxCount;

public ThreadQueue(int threadsMaxCount)
{
ThreadsMaxCount = threadsMaxCount;
}

///
/// Creates a new thread queue with a maximum number of threads and the tasks that should be executed.
///
/// The maximum number of currently active threads.
/// The tasks that should be executed by the queue.
public ThreadQueue(int threadsMaxCount, ThreadStart[] tasks) : this(threadsMaxCount)
{
RaiseCompleteEventIfQueueEmpty = true;
foreach (ThreadStart task in tasks)
{
Queue.Enqueue(task);
}
}

///
/// Starts to execute tasks. Used in conjunction with the constructor in which all tasks are provided.
///
public void Start()
{
CheckQueue();
}

private readonly object addlock = new object();
///
/// Adds a task and runs it if a exec

There is a lot that needs to be taken into consideration when implementing a threaded work queue and I wouldn't recommend doing it manually unless you've got a really good reason because existing solutions are tested and reliable and without very specific design needs and some hardcore coding to go with it you'll likely wind up with something less efficient/powerful than the existing options. That said, as an exercise this can be a very educational challenge and I'll try to review it as such; production relevant notes are below.

Technically it's just a matter of preference, but generally speaking, private member variables don't have capitalized first letters to help differentiate between public members and internal implementation details (typical .NET style is CamelCase for public members and pascalCase for fields/variables/parameters).

It seems you are using the Complete event for two different meanings; you should make this two separate events. I wouldn't bother with RaiseCompleteEventIfQueueEmpty, instead just have an ItemComplete event and a QueueComplete event or something like that and always raise the events; if the caller doesn't care about one or the other (or either) they simply don't attach handlers to them.

There are synchronization issues due to your locking strategy. Because your AddTask and CheckQueue methods use different lock objects it's possible to be adding to and accessing the queue at the same time. Because Queue is not inherently thread-safe this could cause issues. You're better off using a single lock between the two. If you're on framework 4.0 you may want to consider using ConcurrentQueue instead.

BackgroundWorker contains provisions intended to make it easy to setup simple background tasks separate from a UI thread. BackgroundWorker is also a Component, which technically means you should Dispose of it when you're done (not from its own thread) although I don't think its implementation depends on it currently, that could change and you want to follow proper practices regarding IDisposable. It doesn't look like you need much of the convenience it offers and are mainly using it for the completion event, which you could implement yourself pretty easily. You would have more control and less overhead using Thread which should also keep you conscious of everything that's happening in something this low level. Here's a brief discussion on Stack Overflow. Also worth noting: BackgroundWorker uses BeginInvoke internally which executes the work on a ThreadPool thread.

In regard to using ThreadStart, it's just a delegate like any other, but if you are wanting to parallelize work, it's likely that you will want to provide a context or workload to each thread. It's possible to work around this a few ways, including using closures when defining the delegate but that's messy and not suitable for all situations. The Thread class, which uses ThreadStart, also supports ParameterizedThreadStart. Since you're using Invoke you can probably accept any delegate and any number of parameters, perhaps like so:

private readonly Queue> Queue = new Queue>();
private readonly object queueSync = new object();

public void AddTask(F task, params object[] parameters) where F : Delegate
{
    lock (queueSync)
    {
        if (WorkingThreads.Count == ThreadsMaxCount)
        {
            Queue.Enqueue(new KeyValuePair(task, parameters));
        }
        else
        {
            StartThread(task, parameters);
        }
    }
}

private void StartThread(Delegate task, object[] parameters)
{
    WorkingThreads.Add(task);
    BackgroundWorker thread = new BackgroundWorker();
    thread.DoWork += delegate { task.Invoke(parameters); };
    thread.RunWorkerCompleted += delegate { ThreadCompleted(task); };
    thread.RunWorkerAsync();
}

Warnings: this code doesn't take into account my other suggestions and was written ad-hoc in the text editor here so it has not been compiled or tested; the idea is all I'm trying to demonstrate. There are several better ways to make use of generics or the delegate choice in general and you would probably also want to provide the context data in your completion event.

You should probably provide a little more information in your completion event so that the attached code can see what completed (consider one event handler can be used for many threads and even many pools). Use an object that inherits from EventArgs to contain your task (and the parameters if you take that bit of advice).

In regard to Darragh's answer and your comments there: if you would like to leverage the ThreadPool but need to know when work is complete you can use Task Parallel Library which now ships with 4.0 to use Task objects which by default are scheduled using the ThreadPool. Alternatively, if your tasks will be very long running and you still want to run on your own threads or specify your own concurrency regulations you can implement a Task Scheduler an

StackExchange Code Review Q#654, answer score: 21