C# ThreadPool inplementation

I have implemented some methods to make the creation of multithreaded programs easier for me in the future

The requirement was:

• Be able to add as many Tasks as I need to the pool

• Be able to restrict the amount of concurrently running threads

• Be able to set the thread creation speed

• Be able to cancel the thread creation/currently running threads

This is the code i came up with, note I do not know the conventions of Thread safety and design patterns usually used in multithreaded programming in c# or any other language.

This is a first attempt at a topic im interested in, The point of the review is to see if there are improvements I can make to my code and learn some new methods to make it tidier and easier to understand

```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace MTSimplified
{
public class AlreadyStartedException : Exception { public AlreadyStartedException() { } }
public class NotStartedException : Exception { public NotStartedException(){ } }

class ThreadOrganizer
{

private List threadList;
private int maxThreads,creationInterval,finishedThreads,currentThreadIndex;
private bool keepCreating,started;
private CancellationTokenSource cts;

/ entire pool /
public event EventHandler onDone;
public event EventHandler onStopped;

/ per thread /
public event EventHandler onThreadCreate;
public event EventHandler onThreadFinish;

public ThreadOrganizer(int maxThreads, int creationInterval)
{
this.maxThreads = maxThreads;
this.creationInterval = creationInterval;
this.threadList = new List();
this.finishedThreads = 0;
this.currentThreadIndex = 0;
this.cts = new CancellationTokenSource();
this.keepCreating = true;
this.started = false;

I don't have time to write a better implementation of a thread pool, but I'll point out what I see is wrong.

I'm quite sure your implementation does not satisfy the requirements' intention. Where does it say that none of the tasks should be started when added, and then they should only be started all at once (but not really, because of the delay between thread creation)? And once the "thread pool" has been started, you can't add any further tasks? As it stands, I cannot see any real use for it. Not that I can see a real use for a better version either, since you can pretty much do all that with the standard thread pool.

Other issues:

• Each functionWrapper method runs on a separate thread, and increments finishedThreads when complete. This is not thread safe, and you're bound to lose increments. Given that you use this variable in your while loop in startAll(), this is a problem. Perhaps use Interlocked.Increment.

• Speaking of which, finishedThreads, currentThreadIndex, and keepCreating should be marked volatile so that the reads of their values aren't hoisted out of the while loop.

• Creating a new thread for each task is wasteful. The standard thread pool will reutilise threads from the pool to schedule queued tasks, so you don't have loads of threads constantly being newly created.

• I don't believe your implementation meets the requirement "Be able to cancel [...] currently running threads". I can't test it myself but I'm pretty sure that simply calling Cancel() on a CancellationTokenSource when the function you started is already running will do a whole load of nothing, unless you pass the token in to the method, and then continually test it from within the method (in this case, from within the method that gets invoked when function.Invoke() is called in functionWrapper). Of course, if the method being invoked doesn't care about CancellationTokens, then you're plum out of luck - it's not "cancellable" (apart from aborting the thread). Someone may correct me but I believe that's how it is.

• Use PascalCase for method names and public events.

• Put a space after a comma (e.g. in private int maxThreads,creationInterval,finishedThreads,currentThreadIndex;)

• Be consistent: startAll() uses a member field like so: if (started) ... and two lines later uses "this": this.started = true;. Do one or the other.

StackExchange Code Review Q#143150, answer score: 5