-5

Does this create a race condition in this class? In the while loop? Only 2 downloads should be allowed concurrently. The calculations work should not be done concurrently (time sensitive). The question is about this code. I know there are other ways to accomplish this like using ConcurrentQueue.

//class setup
public class FileService : IFileService
{
    private static Queue<string> fileNamesQueue = new Queue<string>();
    private static int concurentDownloads = 0;
    private static bool calcBusy = false;       

...

    //called by a button click event in UI
    public async void Download(string fileName)
    {
        fileNamesQueue.Enqueue(fileName);

        while (fileNamesQueue.Count > 0)
        {
            await Task.Delay(500);
            if (concurentDownloads < 2 && !calcBusy)
                DownloadItem();
        }
    }

//beginning of perform download
public async void DownloadItem()
    {

        concurentDownloads++;

        var fileName = string.Empty;

        if (fileNamesQueue.Count > 0)
        {

            fileNamesQueue.TryDequeue(out fileName);

            //perform calc work but ensure they are not concurrent
            calcBusy = true;
             //do calc work
            calcBusy = false;

            //create download task
...

//concurentDownloads gets decremented after each download completes in a completed event
StackMan
  • 55
  • 7
  • Should probably add [Volatile](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile) to your static variables just to be safe. – John Wu Jan 25 '18 at 18:13
  • @JohnWu You should not be using `volatile` unless you *really* know what you're doing, and have an extremely strong understanding of what it does, and the entire C# memory model. It's *very* easy to use improperly. Writing lock free code is *very, very* hard, and the situations that require it are *very* rare. – Servy Jan 25 '18 at 18:31
  • @Servy Volatile is needed here (as well as `Interlocked` btw). I am not sure why you'd say to avoid it, it's not going to hurt anything. You won't need a lock just to read it as [32-bit reads and writes are atomic](https://stackoverflow.com/questions/11745440/what-operations-are-atomic-in-c). – John Wu Jan 25 '18 at 18:38
  • @JohnWu The code won't work even *with* volatile. You should avoid it because it's making you think that the code will work, when it won't in fact work, and isn't actually helping. The fact that 32 bit reads and writes are atomic doesn't mean that the code will work, and that's because this code relies on more than just 32 bit reads and writes in its critical sections. As a result of that, it most certainly *does* need locking (or other more robust synchronization mechanisms). – Servy Jan 25 '18 at 18:42
  • Start by saying what the threading model is here. Is everything always on the UI thread? – Eric Lippert Jan 25 '18 at 19:12
  • @EricLippert It's not prominent, but in the code it has, "//called by a button click event in UI" – Servy Jan 25 '18 at 19:12
  • Right, but the answer from Steve assumes that multiple threads are running in Download and DownloadItem, and John Wu thinks that volatile helps, and the original poster mentions a concurrent queue, so multiple people are assuming a threading model here. I'd like to know what the original poster thinks their threading model is. – Eric Lippert Jan 25 '18 at 19:15
  • Also I'd like to know why DownloadItem is *public*. If DownloadItem is public then the number of things being downloaded at the same time is controlled by the external caller, and the caller does not have access to concurrentDownloads. This code seems like it is possibly broken by design. – Eric Lippert Jan 25 '18 at 19:18
  • 1
    I'd also like to know why Download and DownloadItem are both void-returning. If they're void returning then no one has any way of knowing when the download completes, unless that is in code not shown. This code seems possibly badly designed, and maybe the design problems ought to be fixed before an analysis of race conditions is undertaken. – Eric Lippert Jan 25 '18 at 19:22
  • The code actually works very well. Just a question of race condition. The download function is called from a UI click event. downloadItem just hasn't been set to private yet. – StackMan Jan 25 '18 at 19:24
  • Download and DownloadItem are both void return because the download is handled elsewhere. concurentDownloads-- is decremented by the download handler at completion. Any other comments on race condition? – StackMan Jan 25 '18 at 19:30
  • @Servy I disagree that OP's code contains critical sections that access anything other than a 32-bit integer. The code in the example all runs on the main thread, the only parallelism occurs when one of the spawned tasks decrements the counter. – John Wu Jan 26 '18 at 21:57
  • @JohnWu As Eric has mentioned, the code is simply too incomplete to comment on its behavior. None of us can't make claims about what is or isn't in code not shown. – Servy Jan 26 '18 at 22:00

2 Answers2

0

Q: does this create a race condition?
A: Yes, totally

imagine this
Thread 1 : Download's Task.Delay(500) concurentDownloads = 0
Thread 1 : enter DownloadItem
Thread 2 : Download's Task.Delay(500) concurentDownloads = 0
Thread 3 : Download's Task.Delay(500) concurentDownloads = 0
Computer LAG
Thread 2 : enter DownloadItem
Thread 3 : enter DownloadItem
Thread 1 : concurentDownloads++ -> concurentDownloads = 0 + 1 = 1
Thread 2 : concurentDownloads++ -> concurentDownloads = 0 + 1 = 1
Thread 3 : concurentDownloads++ -> concurentDownloads = 0 + 1 = 1
Thread 1 : Download Start
Thread 2 : Download Start
Thread 3 : Download Start
.... //concurentDownloads  = 1
Thread 1 : Download Finish
Thread 2 : Download Finish
Thread 3 : Download Finish

download x 3

You don't need to reinvent the wheel for stuffs like this. Semaphore does exactly what you need.

Steve
  • 11,696
  • 7
  • 43
  • 81
  • The if condition in the 'Download' function prevents the DownloadItem function from being called. So concurentDownloads++ in the DownloadItem function is not under a race condition. Right? When run, concurrencyDownloads count only changes between 1 and 2 and never above 2. So not a race condition. Right? – StackMan Jan 25 '18 at 18:50
  • @StackMan unfortunately your blocker wont work. read the scenario again – Steve Jan 25 '18 at 18:53
  • Your scenario is wrong. Thread 1 should be enter 'Download' not 'DownloadItem'. Download does block 'DownloadItem' conditionally. – StackMan Jan 25 '18 at 18:58
  • I don't see a race condition. concurretDownloads works as expected between 1 and 2 and never higher. CalcBusy works as expected and the queue works as expected. Nothing is racing. – StackMan Jan 25 '18 at 19:06
  • @StackMan your concurentDownloads variable will be 1 even though there are 3 concurrent downloads happening. how is that not a race condition – Steve Jan 25 '18 at 19:10
  • There are never 3 (or more) downloads. The downloadItem function cannot be called if 2 items are already being downloaded. The if statement prevents additional downloads. The debug console is logging downloads as they are started. It's never greater than 2. – StackMan Jan 25 '18 at 20:16
  • @StackMan based on what you just said I think you need to read more about race condition. variable++ is not thread safe at all. and checking on a variable that's not thread safe will not protect you from race condition – Steve Jan 25 '18 at 20:33
  • The context of variable++ determines if it's tread safe and the debug results prove that it is. I will stick with the debug and testing results over your conjecture. The proof is in the execution and lack of contention from others besides you. Thanks. – StackMan Jan 25 '18 at 20:56
  • 2
    @StackMan: The idea that *debugging* and *testing* can prove thread safety has been the downfall of a great many developers. **Debugging and testing do not establish safety**. Many race conditions happen in only billion-to-one scenarios, or only on certain architectures, and cannot be observed while debugging or testing. In particular **the debugger changes the timing of certain events with respect to threading**. – Eric Lippert Jan 25 '18 at 22:32
  • @StackMan: Since you have not even yet explained what the threading model of your program is, it is impossible to say whether your program is correct or not. The code you've presented is insufficient to determine its correctness. – Eric Lippert Jan 25 '18 at 22:33
  • The model is based on a mobile app running c# (ios and android). The UI download buttons calls the async download function and sends in the file name. The FileService is a single instance (singleton). – StackMan Jan 25 '18 at 22:48
  • I think the situation is better explained this way. Whenever I have multiple downloads requested (button tapped) I only allow a new download if <2 are already concurrent. When one completes or if both complete I start more (up to 2) When each new one starts I immediately have each one increment the concurrentDownloads counter. So I make them race to increment the counter which they both will but no more than 2 at a time. It's not the typical UNDESIRED race condition. It is by design. – StackMan Jan 25 '18 at 23:31
  • @StackMan the counter increment will not work when more than one thread enter at the same time. ++ is NOT atomic operator, it first reads the current value and then add one it, then assigns it back. So you could have 100 concurrent downloads happening but the counter is still 1. Though its hard to reproduce for click event. It requires big PC lag spikes and fast clicks to make it happen. But the goal is to avoid this even in situations mentioned above. – Steve Jan 26 '18 at 14:47
  • [Interlocked.Increment](https://msdn.microsoft.com/en-us/library/dd78zt0c.aspx) is an atomic operation, but using it requires a lot of care. You can't merely replacing `i++` with `Interlocked.Increment(i)`;, since code like `if(i](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.blockingcollection-1)) – Brian Jan 26 '18 at 15:46
-1

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:

  1. 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.

  2. 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.

John Wu
  • 50,556
  • 8
  • 44
  • 80
  • Your suggestions worked perfectly! There are times when standard conventions such as using concurrentQueue don't provide the best solution. I needed something closer to the bone. Volatile and interlocked allowed this custom concurrent queue to shine! For anyone saying it's bad code... it's just better code that needs a little cleanup like public to private... Thanks Mr. Wu. – StackMan Jan 28 '18 at 16:49