Fixes #172

[harrisi]

I'm not sure if this is an actual fix, but it's difficult to recreate. This allows for multiple writers and readers, so there may be some instances where the highlighted cells are incorrect, but I'm not sure how that would happen. This also should only happen when rapidly visualizing bots, so shouldn't ever be noticeable anyway.

[ryanheath]

This probably will not fix the issue, but it is a small optimization. The [] operation does an Add anyways when the key is not present.

[harrisi]

Can you explain why this won't fix the issue?

[ryanheath]

The original code and your code are doing the same thing under the hood.

    public TValue this[TKey key]
    {
        ...
        set
        {
            bool modified = TryInsert(key, value, InsertionBehavior.OverwriteExisting);
            ...
        }
    }

    public void Add(TKey key, TValue value)
    {
        bool modified = TryInsert(key, value, InsertionBehavior.ThrowOnExisting);
        ...
    }

Apparently multiple threads can alter the dictionary at the same time. Your code has lowered the probability of the exception, but the problem is still out there somewhere.

[harrisi]

Those aren't the same thing, though. The insertion behavior for [] is to overwrite when there is an existing value, whereas Add will throw when there is an existing value. You can see that in the last argument.

[ryanheath]

True, that's why I said your code have lowered the probability, but it has not removed the main cause.
If the key was not present in the dictionary, it still have to be inserted. When multiple threads try to insert, you can still get an exception. Dictionaries are not thread-safe for modifing their inner buckets.

[harrisi]

It actually won't throw with multiple writers. You can wrap that line in a closure passed to Parallel.For with 100 (or whatever) threads, they'll all happily write over each other without throwing.

I could be convinced that the correct way to do this is to use ConcurrentDictionary, which is what I think you're suggesting as correct, but I imagine there could be a hit to performance. Since the error case of this overwriting incorrect values is that a square may be colored differently briefly - too briefly to even be seen - that doesn't seem like a good reason to change more than necessary.

[ryanheath]

Parallel.For is too clever ;) It sometimes will not use multiple threads when the workload is done fast enough.
Try this instead, depending on your system you might want to add more threads or loop longer.
On my system it throws all the time, and never with a ConcurrentDictionary.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

var dict = new Dictionary<int, int>();
//var dict = new ConcurrentDictionary<int, int>();

var ev = new ManualResetEvent(false);
var t1 = Task.Run(FillUp);
var t2 = Task.Run(FillUp);
ev.Set();
await Task.WhenAll(t1, t2);

Console.WriteLine("done!");

void FillUp() 
{ 
    ev.WaitOne();
    try
    {
        for (var i = 0; i < 100; i++) 
        {
            dict[i] = i;
        }
    }
    catch(Exception x)
    {
        Console.WriteLine(x);
    }
}

[harrisi]

I'm not sure if that is an accurate recreation of the code, since the tasks are created and handled differently (Manual vs Auto reset, Task.Run vs Task.Factory.StartNew, and use of Task.WhenAll), but after reading about it more, I think using a ConcurrentDictionary is more correct.

I'm still trying to recreate the error so I can test a fix, though, so if you have any suggestions on how to do that, let me know. :)

[ryanheath]

My sample code was to show that [ ] is not necessarily thread-safe with an ordinary dictionary.
Fixing the actual code might be a lot harder ...

My sample code is altering the dictionary in a tight loop, the actual code is doing, obviously, a lot more.
If fact, just slapping a ConcurrentDictionary on and be done with it, may lead to other exceptions that are lurking under the hood.
Any state that can be changed simultaneously by the two threads should be guarded. The dictionary might be just one of many more. 🤷‍♂️

We should investigate which code paths can be crossed simultaneously by the two threads to get an idea of what can be or should be guarded.