CacheBase / InMemory Storage concurrency #925
Conversation
|
@IbraheemOsama as discussed before, this PR contains changes to ensure thread concurrency. I myself encountered item already added exception with CacheBase. |
Add Created property to InMemoryStorageItem Use Created property when removing additional items from InMemoryStorage
|
@Odonno can you please review the update ? |
| { | ||
| _concurrentTasks.Add(fileName, request); | ||
| } | ||
| _concurrentTasks.TryAdd(fileName, request); |
There was a problem hiding this comment.
Try add without proper handling will sallow the operation, I mean what will happen if try add failed ? we should handle that or propagate the problem
There was a problem hiding this comment.
of course TryRemove should be handled also
Use Index to set the cache request. Use linq to query for expired items
| _concurrentTasks.Remove(fileName); | ||
| } | ||
| } | ||
| _concurrentTasks.TryRemove(fileName, out request); |
There was a problem hiding this comment.
we need to try again if the remove fails
| } | ||
| InMemoryStorageItem<T> tempItem = null; | ||
|
|
||
| _inMemoryStorage.TryRemove(key, out tempItem); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Cool..so it cannot fail due to async reason :)
| } | ||
|
|
||
| _inMemoryStorage.Remove(key); | ||
| _inMemoryStorage.TryRemove(id, out tempItem); |
There was a problem hiding this comment.
Would filename be null?
There was a problem hiding this comment.
If it can this bit is the least of our worries
There was a problem hiding this comment.
I was mentioning the potential fail of TryRemove due to async op but after reading the link you sent, I'm ok this is fine!
There was a problem hiding this comment.
Yes these concurrent types need careful looking into 🙂
|
|
||
| private Dictionary<string, ConcurrentRequest> _concurrentTasks = new Dictionary<string, ConcurrentRequest>(); | ||
| private ConcurrentDictionary<string, ConcurrentRequest> _concurrentTasks = new ConcurrentDictionary<string, ConcurrentRequest>(); | ||
| private object _concurrencyLock = new object(); |
There was a problem hiding this comment.
Please remove this. It's no longer in use.
| { | ||
| lock (this) | ||
| foreach (var key in keys) | ||
| { |
There was a problem hiding this comment.
key can still be null so need some code to check for it
There was a problem hiding this comment.
Good spot thought you meant in cache base. Muppet (me)
|
|
||
| // clears expired items in in-memory cache | ||
| var keysToDelete = new List<object>(); | ||
| DateTime expirationDate = DateTime.Now.Subtract(duration); |
There was a problem hiding this comment.
I really don't want a fight with date time. Local time. Meh
Use ConcurrentDictionary instead of Dictionary in CacheBase
Use ConcurrentDictionary instead of OrderedDictionary in InMemoryStorage