0

I'm looking for someone to verify my understanding of concurrency and memory model in this situation of code. 

public static class ExampleClass
{
    private static ConcurrentDictionary<int, TimeSpan> DeviceThrottles { get; set; } = new ConcurrentDictionary<int, TimeSpan>();

    public static void ExampleFunction(int deviceId)
    {
        TimeSpan timedelay = DeviceThrottles[deviceId];

        if (timedelay.Seconds < 10)
        {
            DeviceThrottles[deviceId] = new TimeSpan(days: 0, hours: 0, minutes: 0, seconds: 10);
        }
    }
}

The application has multiple threads running that can call ExampleFunction. Given the following situation.

Thread 1 calls ExampleFunction(1) and timedelay was only 5 seconds, so DeviceThrottles[deviceId] is set to a new TimeSpan of 10 seconds.

A short time later, Thread 2 calls ExampleFunction(1). Is it possible that the local thread cache could still have the timedelay of only 5 seconds? My understanding is that ConcurrentDictionary's purpose is just thread-safety of a collection, not maintaining the most up to date memory of objects across threads.

If my understanding is correct, should the code to be modified to the following to ensure the TimeSpan value is up-to-date across threads.

public static void ExampleFunction(int deviceId)
{
    lock (lockObj)
    {
        TimeSpan timedelay = DeviceThrottles[deviceId];

        if (timedelay.Seconds < 10)
        {
            DeviceThrottles[deviceId] = new TimeSpan(days: 0, hours: 0, minutes: 0, seconds: 10);
        }
    }
}

With the lock, it seems like the concurrent dictionary loses its appeal.

Justin
  • 6,373
  • 9
  • 46
  • 72
  • `ConcurrentDictionary` internally uses `Volatile.Read` and `Volatile.Write` to get and set values, so no, putting in a `lock` is not required. – Jeroen Mostert Mar 08 '18 at 14:37
  • TimeSpan is a *structure*. When you read it you get a copy of it. The thread that read the 5 sec value still has that 5 sec value. There's no caching involved – Panagiotis Kanavos Mar 08 '18 at 14:39
  • What you say is not possible, but code is still not "thread safe" in a sense that two (or more) threads can enter `if` block at the same time (for the same device id). Whether it is a problem or not - hard to tell (in this sample that seems to be not a problem). – Evk Mar 08 '18 at 14:40
  • ConcurrentDictionary guarantees *it* is thread-safe for modification anyway. It doesn't give any guarantees for its contents. It won't prevent multiple threads from modifying an object's properties for example. – Panagiotis Kanavos Mar 08 '18 at 14:40
  • @JeroenMostert perfect. That is what I needed. – Justin Mar 08 '18 at 14:41
  • Lock is for letting only one thread to use critical section of code at a time whilst `ConcurrentDictionary` lets you use thread-safe operations of reading/writing value to/from the collection. These are two absolutely different things... – Fabjan Mar 08 '18 at 14:41
  • @Evk It would be fine if multiple threads were to enter the function at the same time. I was mostly wondering if the read as Jeroen stated was most up to date. – Justin Mar 08 '18 at 14:42
  • Well not just function, but specifically `if` block, and for the same `deviceId`. I mean in this sample code it doesn't do any harm, but who knows what the real code is. – Evk Mar 08 '18 at 14:47
  • @JeroenMostert I did some further searching and found this thread https://stackoverflow.com/questions/22569274/thread-volatileread-vs-volatile-read which states "A volatile read is not the same as a "fresh read". Why? It is because the memory barrier is placed after the read instruction. That means the actual read is still free to move up or backwards in time. Another thread could write to the address, but the current thread might have already moved the read to a point in time before that other thread committed it." So Maybe the Volatile.Read still won't guarantee a fresh ready from memory? – Justin Mar 08 '18 at 14:56
  • The linked answer is correct; `Volatile.Read` on its own does not guarantee you're certainly getting the latest value, because of how reads and writes can be reordered. More importantly, though, the volatile reads and writes apply only to the *buckets* inside the dictionary, not to the values inside those buckets, so my skimming of the code was too hasty. I'm quite confident the MS engineers did not screw up in the code (as in, their use of memory barriers is correct with respect to maintaining the dictionary's state), but I can't tell at a glance what's up with bucket contents. – Jeroen Mostert Mar 08 '18 at 15:20
  • Note that your code as *posted* has no need for a `lock` anyhow, because its operation is idempotent. That is to say, if a `TimeSpan` is discovered of less than 10 seconds, it is replaced by one with 10 seconds. At worst, two threads may do this instead of one, but other than the update happening twice, this is not observable, as `TimeSpan`s with identical values are indistinguishable. If you had some sort of complex sequence of different values and you needed an absolute ordering in updates, things would be different, and you might very well need explicit synchronization. – Jeroen Mostert Mar 08 '18 at 15:26
  • I think you're taking the issue the wrong way. `Is it possible that the local thread cache could still have the timedelay of only 5 seconds?` The real question is: why would it matter? Imagine that two threads execute the code at the exact same moment. They will both find a TimeSpan of 5 seconds. No amount of `volatile` will save you from that. From there you have two solutions: either it doesn't matter, and your question can be discarded. Or you have some logic based on the fact that both thread get different values, in which case you **need** synchronization – Kevin Gosse Mar 09 '18 at 07:37
  • To quote Brian Gideon on the question about volatile: `Think about this for a minute. What does it even mean for a read to return the latest value anyway? By the time you use that value it might not be the latest anymore. Another thread may have already written a different value to the same address. Can you still call that value the latest?` – Kevin Gosse Mar 09 '18 at 07:40

0 Answers0