Exporting doc types using queues and multithreading

Question

For a while I have been interested in seeing if some tasks work well when split across multiple threads.

On the one project I have been busy with I have been creating a lot of small utility apps, to do single tasks for me, so this morning while modifying one I thought I would give it a try.

What I wrote seems to work, although I am not sure if it is the best way, or recommended way to do something like this, so I want to ask what is wrong with my code and how could it be done better. What it is doing is not really important, and I am not interested if this sort of task should be split up, but more along the lines of how good or bad is the code that I have written.

private static void ExportDocTypes(IEnumerable<string> docTypes)
{
    var queue = new Queue<string>(docTypes);
    var tasks = new List<Task>();
    for (byte i = 0; i < threadCount; i++)  //threadCount is a const set to 4.
    {
        Action a = () =>
        {
            while(queue.Count() > 0) {
                var type = queue.Dequeue();
                ExportDocuments(type, i);  //i is only sent for response.write
            }
        };
        var t = new Task(a);
        t.Start();
        tasks.Add(t);
        System.Threading.Thread.Sleep(10);
    }
    while (tasks.Count() > 0)
    {
        foreach (var t in tasks)
        {
            if (t.IsCompleted)
            {
                t.Dispose();
                tasks.Remove(t);
            }
        }
        System.Threading.Thread.Sleep(1000);
    }
}

Answer 1

Rather than reviewing your specific code -- which at first glance seems very, very buggy -- let me answer your question directly:

What are the best practices with multithreading in C#?

1. Don't. Multithreading is a bad idea. Programs are hard enough to understand already with a single point of control flow in them; why on earth would you want to add a second, third, fourth... ? Do you enjoy tracking down incredibly difficult-to-reproduce, impossible-to-understand bugs? Do you have an encyclopaedic knowledge of the differences between weak and strong memory models? No? Then don't write multithreaded programs! Processes are a perfectly good unit of work; if you have work to do, spawn a process.

2. Oh, so you want to do multithreaded programming anyways. In that case use the highest level of abstraction available. Programming against threads directly means writing programs about workers when you could be writing programs about jobs. Remember, workers are just means to an end; what you really want to get done are the jobs. Use the Task Parallel Library: tasks are jobs, threads are workers. Let the TPL figure out for you how to make efficient use of workers.

3. Wait, let's go back to point one again. Ask yourself do you really need concurrency? With true concurrency we have two or more points of control in a program actually active at the same time. With simulated concurrency the two points of control take turns using the processor, so they are not actually concurrent. Most of the time we end up with simulated concurrency, because there are more threads than there are processors. This leads us naturally to the conclusion that perhaps actual concurrency is not what we need. Much of the time what people really want is asynchrony; concurrency is one way to achieve asynchrony, but it is not the only way. Use the new async/await functionality in C# 5 to represent asynchronous workflows.

4. If you are confused by the previous point, and think wait a minute, there is no way to achieve asynchrony without multiple threads of control in a process, read Stephen Cleary's article There Is No Thread. Don't try to write multithreaded programs until you understand it.

5. At all costs avoid shared memory. Most threading bugs are caused by a failure to understand real-world shared memory semantics. If you must make threads, treat them as though they were processes: give them everything they need to do their work and let them work without modifying the memory associated with any other thread. Just like a process doesn't get to modify the memory of any other process.

6. Oh, so you want to share memory across threads even though doing so is incredibly bug prone? Again, let me go back to use the highest level tool available to you. Those tools were written by experts. You're not an expert. Do you know how to make a lazily-computed value written to a field such that you are absolutely guaranteed that the lazy computation executes no more than once and the field is never observed to be in an inconsistent state no matter what is in any given processor cache? I don't. You probably don't. Joe Duffy does, which is why you should use the Lazy<T> primitive he wrote rather than trying to write your own.

7. Oh, so you want to write your own code that shares memory? Then take the lock for heaven's sake. I see so much buggy code where people get into their heads that locks are expensive and they write this convoluted godawful wrong code that evades taking a lock that would have cost them twelve nanoseconds to just take. Avoiding a lock to save on its cost is like complaining that the electricity takes too long to get to the light bulb when you flip the switch. Locks are a powerful tool for ensuring memory consistency; if you're going to program shared memory at the thread level, you avoid locks at your peril.

8. If you use Thread.Sleep with an argument other than zero or one in any production code, you are possibly doing something wrong. Threads are expensive; you don't pay a worker to sleep, so don't pay a thread to sleep either. If you are using sleeps to solve a correctness issue by avoiding a timing problem -- as you appear to be in your code -- then you definitely have done something deeply wrong. Multithreaded code needs to be correct irrespective of accidents of timing.

Answer 2

You can use Parallel class:

private static void ExportDocTypes(IEnumerable<string> docTypes)
{
    int i = -1;
    Parallel.ForEach(docTypes, docType => ExportDocuments(docType, Interlocked.Increment(ref i)));
}

http://msdn.microsoft.com/en-us/library/system.threading.tasks.parallel_methods%28v=vs.110%29.aspx

Answer 3

I would do something like this:

private static void ExportDocTypes(IEnumerable<string> docTypes)
{
    var queue = new ConcurrentQueue<string>(docTypes);
    var tasks = new List<Task>();

    for (byte i = 0; i < threadCount; i++)  //threadCount is a const set to 4.
    {
        byte iteration = i;
        Task t = Task.Factory.StartNew(() =>
        {
            string type;
            bool queueNotEmpty = queue.TryDequeue(out type);
            while(queueNotEmpty) 
            {
                ExportDocuments(type, iteration);  //i is only sent for response.write
                bool queueEmpty = queue.TryDequeue(out type);
            }
        });
        tasks.Add(t);
    }

    Task.WaitAll(tasks.ToArray());
    tasks.ForEach(task => task.Dispose());
}

Making use of things like ConcurrentQueue as many threads will be simultaneously accessing it. Also making greater use of Linq and static methods on Task to check for completion instead of using Thread.Sleep.

Edit: Also, totally missed this previously, you need to make a local copy of i within the for loop, otherwise i will point to the current value, instead of the captured value at that iteration. Something to be aware of while spawning Tasks in loops.