4 ms·

I have one example where I didn't see a race as an issue. I was transferring data from one thread, to a second thread which would process it after a delay. This

I have one example where I didn't see a race as an issue. I was transferring data from one thread, to a second thread which would process it after a delay. This transfer was done by locking a mutex, then pushing it onto a queue. The second thread does an unsynchronized fetch of the queue length, and then uses that fetched length as a condition in a loop that does a synchronised dequeue of a single item. In this case, the race condition is not a problem, because the only place that data is removed is in that loop, which is a synchronised access. So the worse case for a race here is that the length that's been read is less than the actual length, which is not a problem because that new data will just be handled the next time round.

StillBored 6y ago
Well for the readers (not saying you have a bug), what you describe can fail on any processor that doesn't implement TSO. Due to the fact that the write of your "queue length" may be visible before the queued item is fully visible. Meaning you read garbage from the newly queued item. Worse, even on a TSO architecture, unless you have a way to explicitly guarantee a compiler fence or there is an explicit data dependency the compiler is likely free to hoist the code which updates the queue length ahead of the code doing the queue update. Again meaning the unsyncronized thread reads that there is a new queue item before that queue item is visible to the reader. How critical this ends up being to the program is heavily dependent on the queue data structure and how its being manipulated. But in a simple copy/linked list type situation referencing garbage data is quite likely. A lot of traditional comp sci programs are plenty happy to teach about data synchronization primitives but completely fail to mention that most general API (posix/etc) primitives also have read/write fences implied. Those fences are as important on any processor made in the last two decades as the atomic operations themselves. As generally the implementations also provide both architectural memory barriers as well as compiler level fencing to assure the compiler isn't reordering the primitive with the code it is intended to protect. Going further, many architectures provide unfenced atomics. So, if you decide to code your own atomic_inc/swap/etc you have to also understand the memory model of the target machine before you have actually created something equivalent to what is taught in your average comp-sci department.
- Measter 6y ago
  The code in question basically looks like this(C#): // Called from Thread 1. public void AddData(Data theData) { lock(dataQueue) { dataQueue.Enqueue(theData); } } // Worker for Thread 2. private void ProcessingThread() { while(true) { int length = dataQueue.Count; for (int i = 0; i < length; i++) { Data curData; lock(dataQueue) { curData = dataQueue.Dequeue(); } // Process data. } // Sleep for a short while. } } And all of that is in its own class, where these are the only two functions that access the data queue. So, from what I can see, in the event where the length is updated before the data is actually inserted, Thread 2 is still forced to wait until Thread 1 is finished before it can access it so it's still safe. Though if I'm wrong, please tell me. No one wants garbage data.
  - gpderetta 6y ago
    I don't claim to know the c# memory model, but there is a chance that the compiler could hoist the 'int lenght = dataQueue.Count out of the loop. Also, it is possible that Count is not a member but an accessor and it might return garbage or crash if it sees some inconsistent data. Still, both of the above can be rare occurrences. The reason that you are not seeing no particular issues is because you are basically busy waiting and polling the queue (i.e. the sleep comment); if you replaced the dataQueue.Count with just a random number generator, the code will still work. If you were doing proper signaling instead of sleeping, then your code will get stuck in case of a missed wakeup.
    - Measter 6y ago
      I'm not sure if it would be allowed to hoist `dataQueue.Count` out of the loop. The docs [0] say that multiple concurrent readers are supported, so I'm not sure if the compiler would be allowed to assume anything about the call's return value. You are correct in that `Count` is a property, and it could be doing extra things, but according to the implementation [1] it just returns the value of a private integer. Of course, this is an implementation detail and shouldn't be relied upon, but I'm not sure how much that could be changed. However, you are mistaken about the code continuing to work if `dataQueue.Count` were replaced with an RNG, or if it were otherwise higher than it should be. The `Dequeue` function raises an exception if called on an empty queue. This one is part of the API, not an implementation detail. An uncaught exception here would crash the thread, not just blindly carry on. Of course, I'm no expert, and could just be talking out of my ass. [0] https://docs.microsoft.com/en-gb/dotnet/api/system.collections.generic.queue-1?view=netcore-3.1#thread-safety https://docs.microsoft.com/en-gb/dotnet/api/system.collectio... [1] https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/src/System/Collections/Generic/Queue.cs https://github.com/dotnet/runtime/blob/master/src/libraries/...

StillBored 6y ago

Well for the readers (not saying you have a bug), what you describe can fail on any processor that doesn't implement TSO. Due to the fact that the write of your "queue length" may be visible before the queued item is fully visible. Meaning you read garbage from the newly queued item. Worse, even on a TSO architecture, unless you have a way to explicitly guarantee a compiler fence or there is an explicit data dependency the compiler is likely free to hoist the code which updates the queue length ahead of the code doing the queue update. Again meaning the unsyncronized thread reads that there is a new queue item before that queue item is visible to the reader. How critical this ends up being to the program is heavily dependent on the queue data structure and how its being manipulated. But in a simple copy/linked list type situation referencing garbage data is quite likely. A lot of traditional comp sci programs are plenty happy to teach about data synchronization primitives but completely fail to mention that most general API (posix/etc) primitives also have read/write fences implied. Those fences are as important on any processor made in the last two decades as the atomic operations themselves. As generally the implementations also provide both architectural memory barriers as well as compiler level fencing to assure the compiler isn't reordering the primitive with the code it is intended to protect. Going further, many architectures provide unfenced atomics. So, if you decide to code your own atomic_inc/swap/etc you have to also understand the memory model of the target machine before you have actually created something equivalent to what is taught in your average comp-sci department.

Measter 6y ago
The code in question basically looks like this(C#): // Called from Thread 1. public void AddData(Data theData) { lock(dataQueue) { dataQueue.Enqueue(theData); } } // Worker for Thread 2. private void ProcessingThread() { while(true) { int length = dataQueue.Count; for (int i = 0; i < length; i++) { Data curData; lock(dataQueue) { curData = dataQueue.Dequeue(); } // Process data. } // Sleep for a short while. } } And all of that is in its own class, where these are the only two functions that access the data queue. So, from what I can see, in the event where the length is updated before the data is actually inserted, Thread 2 is still forced to wait until Thread 1 is finished before it can access it so it's still safe. Though if I'm wrong, please tell me. No one wants garbage data.
- gpderetta 6y ago
  I don't claim to know the c# memory model, but there is a chance that the compiler could hoist the 'int lenght = dataQueue.Count out of the loop. Also, it is possible that Count is not a member but an accessor and it might return garbage or crash if it sees some inconsistent data. Still, both of the above can be rare occurrences. The reason that you are not seeing no particular issues is because you are basically busy waiting and polling the queue (i.e. the sleep comment); if you replaced the dataQueue.Count with just a random number generator, the code will still work. If you were doing proper signaling instead of sleeping, then your code will get stuck in case of a missed wakeup.
  - Measter 6y ago
    I'm not sure if it would be allowed to hoist `dataQueue.Count` out of the loop. The docs [0] say that multiple concurrent readers are supported, so I'm not sure if the compiler would be allowed to assume anything about the call's return value. You are correct in that `Count` is a property, and it could be doing extra things, but according to the implementation [1] it just returns the value of a private integer. Of course, this is an implementation detail and shouldn't be relied upon, but I'm not sure how much that could be changed. However, you are mistaken about the code continuing to work if `dataQueue.Count` were replaced with an RNG, or if it were otherwise higher than it should be. The `Dequeue` function raises an exception if called on an empty queue. This one is part of the API, not an implementation detail. An uncaught exception here would crash the thread, not just blindly carry on. Of course, I'm no expert, and could just be talking out of my ass. [0] https://docs.microsoft.com/en-gb/dotnet/api/system.collections.generic.queue-1?view=netcore-3.1#thread-safety https://docs.microsoft.com/en-gb/dotnet/api/system.collectio... [1] https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/src/System/Collections/Generic/Queue.cs https://github.com/dotnet/runtime/blob/master/src/libraries/...