Potential concurrency bugs in FunctionsView

[stephentoub]

FunctionsView appears to be trying to be thread-safe, as its SemanticFunctions and NativeFunctions are both ConcurrentDictionary. However, in that case there are a couple of concurrency bugs lurking.

First, while the dictionaries are ConcurrentDictionary, the values stored for each key are List<T>. The code for AddFunction looks like this:

            if (!this.SemanticFunctions.ContainsKey(view.SkillName))
            {
                this.SemanticFunctions[view.SkillName] = new();
            }

            this.SemanticFunctions[view.SkillName].Add(view);

If multiple threads concurrently try to add a FunctionView for the same skill, they could end up both mutating a List<T> concurrently, which is not safe.

Second, with that same code, if multiple threads both try to add a FunctionView for the same skill, and if both see that the SemanticFunctions doesn't yet contain that skill name, they will race to create the list to store for that skill. While the dictionary itself won't be corrupted from a data structure perspective, one thread might add its list and then proceed to add to the list, and the other thread might then (having already checked ContainsKey and found it to return false) overwrite the original list with a new one, there by losing the data the other thread wrote. Such initialization should be done using either TryAdd or GetOrAdd so that the dictionary provides the synchronization to ensure nothing is overwritten.

These issues can easily be fixed by using GetOrAdd and locking on the List<>... except the collections are exposed publicly from the FunctionsView, which means anyone else accessing these would also need to employ the same synchronization scheme. That suggests the API should be designed differently.

(And related to these being exposed publicly, the properties also have public setters... if someone were to set these concurrently with other operations being performed, that could lead to other failure modes, e.g. because lookups are being done first by calling ContainsKey and then separately by using the indexer, the collection could actually be changed to a completely different instance between those calls.)

cc: @dluc, @shawncal

[dluc]

I think we should change FunctionsView to protect the internal data, allowing only to add and list, not remove, and potentially handle conflicts. Plus needs testing and clear specs about multithreaded access and expectations