Repository navigation
GetOrCreateExclusive() and GetOrCreateExclusiveAsync(): Exclusive versions of GetOrCreate() and GetOrCreateAsync() #36500
Description
It would be nice with equivalents of GetOrCreate() and GetOrCreateAsync() where it is ensured that the factory is only invoked once for each cache miss.
This basically resolves aspnet/Caching#218, but with alternate extension methods.
An implementation proposal:
public static TItem GetOrCreateExclusive<TItem>(this IMemoryCache cache, object key, Func<ICacheEntry, TItem> factory)
{
object result;
if (!cache.TryGetValue(key, out result))
{
SemaphoreSlim exclusiveLock;
lock (cache)
{
exclusiveLock = cache.GetOrCreate("__exclusive__", entry => {
entry.Priority = CacheItemPriority.NeverRemove;
return new SemaphoreSlim(1);
});
}
exclusiveLock.Wait();
try
{
if (cache.TryGetValue(key, out result))
{
return (TItem)result;
}
var entry = cache.CreateEntry(key);
result = factory(entry);
entry.SetValue(result);
entry.Dispose();
}
finally
{
exclusiveLock.Release();
}
}
return (TItem)result;
}
public static async Task<TItem> GetOrCreateExclusiveAsync<TItem>(this IMemoryCache cache, object key, Func<ICacheEntry, Task<TItem>> factory)
{
object result;
if (!cache.TryGetValue(key, out result))
{
SemaphoreSlim exclusiveLock;
lock (cache)
{
exclusiveLock = cache.GetOrCreate("__exclusive__", entry => {
entry.Priority = CacheItemPriority.NeverRemove;
return new SemaphoreSlim(1);
});
}
exclusiveLock.Wait();
try
{
if (cache.TryGetValue(key, out result))
{
return (TItem)result;
}
var entry = cache.CreateEntry(key);
result = await factory(entry); //.ConfigureAwait(false);
entry.SetValue(result);
entry.Dispose();
}
finally
{
exclusiveLock.Release();
}
}
return (TItem)result;
}
Activity
rmja commentedon Oct 19, 2016
In the async version it should of cause be
await exclusiveLock.WaitAsync();and not the sync equivalent.ttrider commentedon Nov 11, 2016
Looking at the MemoryCache.cs implementation, I wonder how bad would it be to use ConcurrentDictionary there instead of plain Dictionary. and expose the GetOrAdd method directly.
I know that the interface doesn't have it and I would really hate IMemoryCache2, but at this point, I'm seriously considering forking implementation of the MemoryCache nd making change myself...
JunTaoLuo commentedon Nov 15, 2016
@ttrider We are looking into updating the implementation of
MemoryCacheto useConcurrentDictionaryin dotnet/extensions#242. Note that theGetOrAddmethod does not guarantee, by itself, that the factory is only invoked once and the implementation will involveLazy<T>.ttrider commentedon Nov 15, 2016
Hello John,
This is the great news! While we wait on your implementation, for a time being, we created the following poor-man wrapper implementation.
https://gist.github.com/ttrider/a5ae8fc86ccfe6a4243f4481a5858a80
I think I’ll rename my methods to match your signatures – it will make dropping and replacing them easier.
Thank You,
Vladimir
glennc commentedon Feb 9, 2017
I don't think this can fit into 2.0 right now. It needs a fairly large amount of investigation and testing to make it work well.
ohadschn commentedon Jul 8, 2017
It would be great if in the async version, expiry could be set as a function of the retrieved value (see https://github.com/aspnet/Caching/issues/338).
amit-ezra commentedon Mar 25, 2018
@JunTaoLuo
it looks like the ConcurrentDictionary is already implemented, BUT the GetOrAdd method of the memory cache still isn't thread safe, even when using Lazy. it still doesn't use the back line ConcurrentDicionary.GetOrAdd...
I think it's very misleading have a non-atomic (non thread safe) GetOrAdd
is there a direct issue just on the GetOrAdd?
jelical commentedon May 6, 2018
2 proposed solutions:
Based on Lazy - https://gist.github.com/jelical/17a3e1ef03af2901f4eb6866291239c6
Based on interlocks - https://gist.github.com/jelical/c424003efe008435b49fd54a3d1bd5b0
Both solutions are tested under heavy load, working with relatively the same performance, supporting multiple IMemoryCache instances and guarantee exclusive fetch call.
Updated: added consistent exceptions propagation
dengere commentedon Jun 23, 2018
@jelical welldone, thanks. Could you please share that your test code?
BladeWise commentedon Feb 18, 2019
@jelical Considering that
ConcurrentDictionary<K, V>.GetOrAdddoes not guarantee that the factory method is called once, as per documentation, wouldn't these method have the same issues as usingLazy<Task<T>>directly as cached values?As far as I can see, using
Lazy<Task<T>>andGetOrCreateis far better than usingGetOrCreateAsyncdirectly, because while the latter delays cache item addition by the time required to construct the cached value (making the contention window wider), the former creates the lazy object delaying creation for when the item is used (so the contention window is the smallest possible). So, even if theLazy<Task<T>>mitigates the issue, race conditions could still occur. Am I wrong?slavanap commentedon May 11, 2019
Greetings,
Here's my solution for your issue.
https://gist.github.com/slavanap/3b9c436c2defae8b1d305ad30fdfe889
It's based on one
ConcurrentDictionarythat storesTaskCompletionSourcefor every new cache value being created.It uses more abstracted
IDistributedCache(instead ofIMemoryCache) as a storage, and I can rewrite it forMemoryCacheif needed.Dotnet-GitSync-Bot commentedon May 15, 2020
I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.
theodorzoulias commentedon Jan 14, 2021
Related StackOverflow question: Stop Reentrancy on MemoryCache Calls
deeprobin commentedon Mar 31, 2022
For completeness. If I understand correctly, the following is proposed here:
An implementation example has already been given by @rmja in top post.
I myself have not worked much with
Microsoft.Extensions.Caching.Memory. Therefore I cannot say much about it.@maryamariyan Is this a relevant API? If yes, you could take a look at it (this issue is already a bit older than 5 years).
maryamariyan commentedon Apr 6, 2022
GetOrCreateis not thread safe and for that to become possible there would need to be lock statements added into MemoryCache and due to perf regression concerns I don't think we will go with that change fix this thread safety issue.I don't think we would want these extension methods added built-in to the library. Doesn't seem like a bigger value add than just letting whomever needs this functionality to just add it directly into their code. Would make sense to keep the onus on the app side to make sure they keep their transactions thread safe by also assuring they don't make calls to
GetOrCreate, otherwise as explained in this issue it could lead to factory func inGetOrCreategetting called multiple times, etc. for a given cache miss in a multithreaded scenario.Closing as won't fix.