[Solved] For a concurrent download handler does this create a race condition?

Question

As you can tell from some of the comments, multithreaded code is serious business. It’s best to follow a proven pattern, e.g. producer-consumer, or at least use an established synchronization primitive like a monitor or semaphore. Because I might be wrong. It is easy to go over code that you came up with yourself and miss miss common multithreading concerns, and I’m not immune to it. I’ll probably get some downvotes just for commenting without seeing the rest of your solution.

That being said, almost all of the code that you have provided is not multithreaded code. Please correct me if I’m wrong, but it all runs on the main thread* (well await Task.Wait could put your code on a different thread for its continuation, depending on your synchronization context, but it’ll be given the same threading context). The only parallel processing that occurs comes from the Task that you create to download the file (you didn’t even include that bit of the code) and the only data element that is subject to parallel access is int concurrentDownloads. Because of this, you can get away with using the Queue (and not a ConcurrentQueue); also, I’m not sure what the purpose of calcBusy is because it seems like it is not needed.

The one thing you have to worry about is that concurrent access to int concurrentDownloads, and I do think I see a couple of problems, although they won’t necessarily manifest themselves on your O/S and chipset and with your build, although if they did it might be so intermittent that you may not realize until your code has gone to production. Here are the changes I would make:

Mark concurrentDownloads as Volatile. If you don’t, the compiler might optimize out the variable check entirely, since it may not realize that it is going to be modified on other threads. Also, the CPU might optimize out main memory access, resulting in your threads having different copies of the variable. On Intel you are safe (for now) but if you were to run your code on certain other processors, it may not work. If you use Volatile, the CPU will be given special instructions (memory barriers) to ensure caches are flushed appropriately.
Use Interlocked.Increment(ref int) and Interlocked.Decrement(ref int) to modify the counter. If you don’t, two threads might attempt to modify the value at the same time, and if the chipset doesn’t support atomic increments, one of the modifications could get lost.

Also, make sure you are actually awaiting your Tasks and handling any exceptions. If a task ends up with an unhandled exception, I believe it’ll get thrown when the task is garbage collected, and unless you have something to catch it, it’ll crash your whole process.

Accepted Answer

As you can tell from some of the comments, multithreaded code is serious business. It’s best to follow a proven pattern, e.g. producer-consumer, or at least use an established synchronization primitive like a monitor or semaphore. Because I might be wrong. It is easy to go over code that you came up with yourself and miss miss common multithreading concerns, and I’m not immune to it. I’ll probably get some downvotes just for commenting without seeing the rest of your solution.

That being said, almost all of the code that you have provided is not multithreaded code. Please correct me if I’m wrong, but it all runs on the main thread* (well await Task.Wait could put your code on a different thread for its continuation, depending on your synchronization context, but it’ll be given the same threading context). The only parallel processing that occurs comes from the Task that you create to download the file (you didn’t even include that bit of the code) and the only data element that is subject to parallel access is int concurrentDownloads. Because of this, you can get away with using the Queue (and not a ConcurrentQueue); also, I’m not sure what the purpose of calcBusy is because it seems like it is not needed.

The one thing you have to worry about is that concurrent access to int concurrentDownloads, and I do think I see a couple of problems, although they won’t necessarily manifest themselves on your O/S and chipset and with your build, although if they did it might be so intermittent that you may not realize until your code has gone to production. Here are the changes I would make:

Mark concurrentDownloads as Volatile. If you don’t, the compiler might optimize out the variable check entirely, since it may not realize that it is going to be modified on other threads. Also, the CPU might optimize out main memory access, resulting in your threads having different copies of the variable. On Intel you are safe (for now) but if you were to run your code on certain other processors, it may not work. If you use Volatile, the CPU will be given special instructions (memory barriers) to ensure caches are flushed appropriately.
Use Interlocked.Increment(ref int) and Interlocked.Decrement(ref int) to modify the counter. If you don’t, two threads might attempt to modify the value at the same time, and if the chipset doesn’t support atomic increments, one of the modifications could get lost.

Also, make sure you are actually awaiting your Tasks and handling any exceptions. If a task ends up with an unhandled exception, I believe it’ll get thrown when the task is garbage collected, and unless you have something to catch it, it’ll crash your whole process.