Now this is a nice bug that was found accidentally by ArchiBoT...
ReaderWriterLockSlim() is very decent solution, but it's thread-based, and we're using our ConcurrentHashSet in mixed async/sync context. This means that if we use something like:
foreach (var item in concHashSet) {
await AnythingAsync().ConfigureAwait(false);
}
It's totally possible that we'll request read lock as thread 1, and release the read lock as thread 2, which will lead to RWLock exception => System.Threading.SynchronizationLockException: The read lock is being released without being held.
Fortunately it looks like we didn't have any scenario like this in ASF, as this was possible only when we async/await while enumerating over ConcurrentHashSet, so that specific bug didn't affect ASF codebase (yet). Still, I must fix this as current implementation is not thread-safe, so our HashSet is in fact not concurrent in the first place.
I analyzed possible solutions and there are basically 3: either using ConcurrentDictionary and wrapping around it, replacing lock with SemaphoreSlim, or using third-party AsyncReaderWriterLock from StephenCleary. SemaphoreSlim entirely kills the concept of multiple readers one writer, and could affect performance negatively, moreover - it doesn't support upgreadable lock scenario we have with ReplaceIfNeededWith(). Concurrent dictionary would be nice if I didn't have that awful memory hit from storing mandatory pointless value, plus I don't really like concept of wrapping around conc dictionary if I can simply use it right away and drop my conc hashset entirely. AsyncReaderWriterLock seem to be really well written, and works on Mono + should be compatible with .NET core in the future, so we should go for it as it's the best bet both performance-wise and memory-wise.
This brings another package dependency and changes a bit backend of ConcurrentHashSet
Time to enforce some common file layout, as general mess started to annoying me. Sorry in advance for people using custom forks and having merge conflicts, this will help everybody in long-run
I really like this approach, as it has only one caveat: memory usage.
We need to keep in memory list of all packages that our account owns, which will result of N * 32bit extra memory usage, where N is equal to number of package licenses the account owns. This results in around 16 KB extra memory usage for my 4k account.
However, apart from that there are no real caveats as checking if we own given packageID is O(1) operation, and I think that apart from this extra memory footprint there can be more benefits of having this field in future, besides, my 16 KB is extreme case, as usually nobody goes that high. Ship it!
