an old bug and learning something new

[JohnH]

I have for some been getting weird exceptions from code to add items to a dictionary like this:

If Not dct.ContainsKey(item.Key) Then
    dct(item.Key) = New List(Of String)
End If

Once every million operations I'm sure I got NullReferenceException before (previous VS version), but could not pinpoint it, neither dictionary, item nor item.Key was Nothing. This was happening in multi-threading with UI ties that maybe wasn't thought through. I ignored it then with a Try-Catch that didn't seem to cause more trouble. Today I got a different exception from same code, the KeyNotFoundException amazingly. I finally figured I should add a synchronization lock and stumbled upon ConcurrentDictionary to handle that. I've never used this class before, it was new with .Net 4. So changed the code to:

dct.TryAdd(item.Key, New List(Of String))

This also made the ContainsKey check redundant. I read that:

All public and protected members of ConcurrentDictionary<TKey,TValue> are thread-safe and may be used concurrently from multiple threads. However, members accessed through one of the interfaces the ConcurrentDictionary<TKey,TValue> implements, including extension methods, are not guaranteed to be thread safe and may need to be synchronized by the caller.

I was wondering if using the default Item property also was thread-safe:

dct(item.Key) = New List(Of String)

According to ConcurrentDictionary<TKey,TValue>.Item[TKey] Property (System.Collections.Concurrent) | Microsoft Docs this implements IDictionary<TKey,TValue>.Item[TKey] Property - so "not guaranteed". Looking at the code in .Net Reflector shows that this code actually is equivalent of the thread-safe AddOrUpdate method. I think the documentation could have mentioned this. Anyway if I wanted to use this code instead, and it should work like before, I would have to add the ContainsKey check again, so TryAdd is better for me in this case. Maybe the standard Dictionary also should get these methods? Reducing 3 lines of code to 1 here and there can do something for readability.

In end I removed a few lines of code, removed a threading bug that happened very rarely, and I think things runs more smoothly now. It's funny though, the weird exceptions that it caused and how difficult it was to figure out. Who would have thought that "dct(key) = value" would throw KeyNotFoundException (or nonsensical NullReferenceException), and that the cause was some kind of threading violation?

The program was written more than 9 years ago, I've been thinking to rewrite it too one day. It's actually a program I use daily and that has worked well as it is.