#41458, async cache revoke (in ahead) #233
Conversation
| public class ReverseItem | ||
| { | ||
| public HashSet<string> CacheKeysSet = new HashSet<string>(); | ||
| public DateTime WhenRevoked = DateTime.MinValue; |
There was a problem hiding this comment.
why magic number DateTime.MinValue instead of DateTime?
| { | ||
| public class ReverseItem | ||
| { | ||
| public HashSet<string> CacheKeysSet = new HashSet<string>(); |
There was a problem hiding this comment.
CacheKeysSet is use by one thread at a time?
| @@ -0,0 +1,49 @@ | |||
| using System; | |||
| public ICollection<Item> Dequeue(DateTimeOffset olderThanOrEqual) | ||
| { | ||
| var oldItems = new List<Item>(); | ||
| lock (_queue) |
There was a problem hiding this comment.
Why do we need this lock?
Can we active it with out locking?
| @@ -81,10 +81,9 @@ public Task<List<T>> WhenEventsReceived(int? expectedNumberOfEvents, TimeSpan? t | |||
|
|
|||
| var cancel = new CancellationTokenSource(timeout.Value); | |||
There was a problem hiding this comment.
it's not relay related but we should try to reduce the use of multiple timers for better performance
|
|
||
| private readonly CancellationTokenSource _cleanUpToken; | ||
| private readonly TimeBoundConcurrentQueue<string> _revokesQueue = new TimeBoundConcurrentQueue<string>(); | ||
| internal ConcurrentDictionary<string, ReverseItem> RevokeKeyToCacheKeysIndex { get; set; } = new ConcurrentDictionary<string, ReverseItem>(); |
There was a problem hiding this comment.
can be made private?
| var arrayOfCacheKeys = cacheKeys.ToArray();// To prevent iteration over modified collection. | ||
| if (shouldLog && arrayOfCacheKeys.Length==0) | ||
| Log.Info(x => x("There is no CacheKey to Revoke", unencryptedTags: new { revokeKey })); | ||
| _revokesQueue.Enqueue(now, revokeKey); |
There was a problem hiding this comment.
For modifications and write operations to the dictionary, ConcurrentDictionary<TKey,TValue> uses fine-grained locking to ensure thread safety. (Read operations on the dictionary are performed in a lock-free manner.) However, the valueFactory delegate is called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, GetOrAdd is not atomic with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class.
Since a key/value can be inserted by another thread while valueFactory is generating a value, you cannot trust that just because valueFactory executed, its produced value will be inserted into the dictionary and returned. If you call GetOrAdd simultaneously on different threads, valueFactory may be called multiple times, but only one key/value pair will be added to the dictionary.
There was a problem hiding this comment.
is it ok to call _revokesQueue.Enqueue(now, revokeKey);
multiple times?
| } | ||
| else | ||
| // Lock wide while processing ALL the keys, to compete with possible call (and insertion to cache) to be in consistent state | ||
| lock (rItem) |
There was a problem hiding this comment.
I don't like this lock ->
When you do MemoryCache.Remove(cacheKey) it taking inside the same lock in on RevokeKeyToCacheKeysIndex item but in the call back of the remove with not relay gentry where it will be running and can cause a dead lock in other dot net implementations or ege case
| } | ||
| } | ||
| catch (TaskCanceledException) | ||
| { |
There was a problem hiding this comment.
add health check on other Exception
There was a problem hiding this comment.
Check with IHealthMonitor.
| if (RevokeKeyToCacheKeysIndex.TryGetValue(revokeKey.Data, out var reverseItem)) | ||
| if (!reverseItem.CacheKeysSet.Any()) | ||
| // We compete with possible call, adding the value to cache, exactly when dequeue-ing values | ||
| lock (reverseItem.CacheKeysSet) |
There was a problem hiding this comment.
lock should be on reverseItem
No description provided.