Risky use of ConcurrentDictionary

[bpierce88]

Hi -- just looking at recent changes, I happened to notice this on line 52 of ExcelDna.Integration/ExcelRtd.cs:

registeredRtdServerTypes[progIdAtt.Value] = rtdType;

...but that is not a threadsafe usage. It should be this:

registeredRtdServerTypes.AddOrUpdate(progIdAtt.Value, rtdType, (k, v) => rtdType);

...or this if the value is only set once:

registeredRtdServerTypes.TryAdd(rtdType);

[govert]

@Sergey-Vlasov Could you please check this in the context of the RTD threadsafe support?

[Sergey-Vlasov]

According to the documentation https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=net-8.0#remarks

"To do this: Store a key/value pair in the dictionary unconditionally, and overwrite the value of a key that already exists Use this method: The indexer's setter: dictionary[key] = newValue"

"All these operations are atomic and are thread-safe with regards to all other operations on the [ConcurrentDictionary<TKey,TValue>] class."

Thus my understanding that our implementation after https://github.com/Excel-DNA/ExcelDna/pull/658 is thread-safe.

[bpierce88]

Hi -- sorry, you are correct. Such operations are safe in the sense that they are effectively atomic so will not corrupt the state of the dictionary. Since the value being inserted is a type ...so not something where you'd be setting any state... there's really no risk here.

The risk comes from cases like this:

someCurrentDictionary[key].SomeState = x;

...where there could be subtle bugs from the object in the dictionary not having the state set ...because some other thread inserted a new value for that key after it was retrieved. That's not the case here, so this should be closed.