NH-3885 - Replace ThreadSafeDictionary wrapper with Framework's ConcurrentDictionary #498
Conversation
| @@ -247,7 +248,7 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings | |||
| if (cache != null) | |||
| { | |||
| caches.Add(cacheRegion, cache); | |||
| allCacheRegions.Add(cache.RegionName, cache.Cache); | |||
| ((IDictionary<string, ICache>)allCacheRegions).Add(cache.RegionName, cache.Cache); | |||
There was a problem hiding this comment.
I think the explicit interface implementations of the ConcurrentDictionary are not thread safe (that's why they are explicitly implemented). I had a similar solution on my machine. Currently I'm in holidays. I'll compare yours to mine when I'm back.
There was a problem hiding this comment.
@stefan-steinegger I had that same concern too, but I looked at the Microsoft Reference Source and the IDictonary implementation is a very simple wrapper around the main TryAdd and it also throws if not added (TryAdd returns false). This throw code could easily be moved into the application, but since it is in the library, in the explicit IDictionary implementations, I left it there.
There was a problem hiding this comment.
The documentation, however, states:
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.
We shouldn't rely on behavior the documentation doesn't promise.
There was a problem hiding this comment.
Shouldn't we simply use TryAdd() instead?
with Framework's ConcurrentDictionary.
| ((IDictionary<string, ICache>)allCacheRegions).Add(cache.RegionName, cache.Cache); | ||
| if (!allCacheRegions.TryAdd(cache.RegionName, cache.Cache)) | ||
| { | ||
| throw new HibernateException("Key already existed"); |
There was a problem hiding this comment.
As per guidelines, exception messages are supposed to be proper sentences, which includes a period at the end. (Multiple locations.)
Only use ConcurrentDictionary through the public methods, not the interface methods.
|
@oskarb I updated the messages to a bit more than the standard .Net framework Dictionary exception sentence, including the dictionary variable name. |
|
I'm doing some tests with the new version and it seems NHibernate.Envers relies on the ThreadSafeDictionnary class. In the meantime I've created a pull request for envers. |
This should address NH-3885 by replacing all uses of the custom
ThreadSafeDictionarywrapper with the .NET Framework providedConcurrentDictionary.In most places the
IDictionary<,>interface is kept as it provides the expected exception throwing when adding existing keys, otherwise the caller would be responsible for error handling on the result ofTryAdd().In
SessionFactoryImplspecifically, I update the code to use the thread-safeGetOrAdd()as that was what the intention of the code was.This removes
ThreadSafeDictionaryfrom the project.