Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetOrCreateAync for IDistributedCache #36379

Open
avin-kavish opened this issue May 30, 2019 · 12 comments
Open

GetOrCreateAync for IDistributedCache #36379

avin-kavish opened this issue May 30, 2019 · 12 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Caching
Milestone

Comments

@avin-kavish
Copy link

avin-kavish commented May 30, 2019

Is your feature request related to a problem? Please describe.

Maybe I've missed it, but I searched the API docs. It felt a bit odd not finding this.
It is often a common pattern to fetch a value from a cache or to compute and set the value if the cache does not contain the value. For this purpose the IMemoryCache has an extension in the form CacheExtensions.GetOrCreateAsync. In-memory caches are practically limited and most production scenarios use a distributed cache of some form or the other. But IDistributedCache does not seem to contain a similar extension.

Describe the solution you'd like

The IMemoryCache extension has the following signature

public static System.Threading.Tasks.Task<TItem> GetOrCreateAsync<TItem> (this Microsoft.Extensions.Caching.Memory.IMemoryCache cache, object key, Func<Microsoft.Extensions.Caching.Memory.ICacheEntry,System.Threading.Tasks.Task<TItem>> factory);

Similarly, I have created my own set of IDistributedCache extensions for this purpose, one of which has the signature,

public static async Task<T> GetOrCreateAsync<T>(this IDistributedCache cache, 
string key, Func<Task<T>> creator, DistributedCacheEntryOptions options = null)

I propose adding this or perhaps one or more of the following variants to the upstream project.

public static async Task<T> GetOrCreateAsync<T>(this IDistributedCache cache, 
Func<IDistributedCacheEntry<T>, Task<T>> factory)

where IDistributedCacheEntry : { string Key; T Value; .....Expiration Options }

public static async Task<T> GetOrCreateAsync<T>(this IDistributedCache cache, string key, Func<Task<T>> factory)

public static async Task<string> GetOrCreateAsync(this IDistributedCache cache, string key, Func<Task<string>> factory)

public static async Task<byte[]> GetOrCreateAsync(this IDistributedCache cache, string key, Func<Task<byte[]>> factory)

IDistributedCache already has string and byte overloads for Get and Set so it only seems appropriate for GetOrCreate to do the same.

Describe alternatives you've considered

The developer can always do

if(Get() == null)
{
   ComputeValue();
   SetValue();
}

But this pattern gets repeated so much that it warrants it's own method. It also introduces a branched condition and a closure under the if statement which increases the cyclomatic complexity of the (action) method, requiring variables to be hoisted and such. eg: In totally not psuedo CJavaSharp++

Model model;
model = cache.Get('key')
if(model == null)
{
  model = ComputeValue();
  cache.Set('key', model);
}
return view(model);

with the proposed extension:

var model = await cache.GetOrCreateAsync('key', 
  async () => await TableScanorAPICallorSomeDelayInducingComputation())

also enabling this on rare occasions:

public async Task<IActionResult> Action() => View(await GetOrCreateAsync('key', async () => await TableScanorAPICallorSomeDelayInducingComputation()));

Real World Usage Example

This is how I use my personal extensions:

    var vm = await _cache.GetOrCreateAsync("default-home-page-data", 
      async () => new HomePageViewModel
      {
        Years = await _catalog.YearsWithModelsAsync(),
        Makes = await _catalog.PopularMakesAsync(),
        Categories = await _catalog.CategoriesAtDepthAsync(1),
        Stores = await _merchant.TopStoresAsync(),
        FeaturedListings = await _listing.FeaturedListingsAsync()
       });

    var user = await _cache.GetOrCreateAsync("summary-user-" + userId, 
      () => _userM.GetUserAsync(User), 
      new DistributedCacheEntryOptions()
        .SetAbsoluteExpiration(TimeSpan.FromMinutes(5)));

Real World Implementations in Alternate Frameworks

If you can't take my word for it, Rails already has this implementation as

Rails.cache.fetch("#{cache_key}/competing_price", expires_in: 12.hours) do
      Competitor::API.find_price(id)
    end

Where the return value of the do block is cached and returned if the fetch misses.

@mkArtakMSFT mkArtakMSFT transferred this issue from dotnet/aspnetcore May 30, 2019
@analogrelay
Copy link
Contributor

Generally, a distributed cache can't give the same kind of behaviors that an in-memory cache can provide here, so we don't provide a built-in GetOrCreateAsync method. Also it would a major breaking change since we'd be adding a method to an interface, and Extensions needs to run on .NET Standard 2.0 which doesn't support Default Interface Members.

We don't plan to do this, but it sounds like your extension method is working for you!

@avin-kavish
Copy link
Author

avin-kavish commented Jun 27, 2019

Generally, a distributed cache can't give the same kind of behaviors that an in-memory cache can provide here, so we don't provide a built-in GetOrCreateAsync method.

Can you ungenarilize this for me please? Can you elaborate on why you say a distributed cache cannot support this? Why can't a distributed cache provide a get or create method if it supports both get and create individually? Is this due to the expectation of atomicity? or that the method has to be a distributed cache provider primitive?

Also it would a major breaking change since we'd be adding a method to an interface, and Extensions needs to run on .NET Standard 2.0 which doesn't support Default Interface Members.

Can't it be an extension method instead of part of the interface definition?

@analogrelay
Copy link
Contributor

In investigating further, it does appear that our existing memory cache GetOrCreateAsync doesn't provide full atomicity (since multiple instances of the creation delegate can run simultaneously and overwrite each other), so we could probably support this here. I'll re-open. We don't have a concrete plan to do this yet though, as we have quite a lot of other high-priority issues on our plate.

Can't it be an extension method instead of part of the interface definition?

Yes, it certainly can be an extension method. However, that completely locks out the ability for a distributed provider to override the behavior. For memory caches, we decided this was OK. I'm not totally sure that's the same for distributed caches, but it's certainly an option.

@analogrelay analogrelay reopened this Jun 27, 2019
@MisinformedDNA
Copy link
Contributor

This is something that people commonly want to do in a cache (per Google search).

@ghost
Copy link

ghost commented May 8, 2020

As part of the migration of components from dotnet/extensions to dotnet/runtime (aspnet/Announcements#411) we will be bulk closing some of the older issues. If you are still interested in having this issue addressed, just comment and the issue will be automatically reactivated (even if you aren't the author). When you do that, I'll page the team to come take a look. If you've moved on or workaround the issue and no longer need this change, just ignore this and the issue will be closed in 7 days.

If you know that the issue affects a package that has moved to a different repo, please consider re-opening the issue in that repo. If you're unsure, that's OK, someone from the team can help!

@MisinformedDNA
Copy link
Contributor

Still interested.

@ghost
Copy link

ghost commented May 9, 2020

Paging @dotnet/extensions-migration ! This issue has been revived from staleness. Please take a look and route to the appropriate repository.

@analogrelay analogrelay transferred this issue from dotnet/extensions May 13, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Caching untriaged New issue has not been triaged by the area owner labels May 13, 2020
@analogrelay analogrelay added this to the Future milestone May 13, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@maryamariyan maryamariyan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 7, 2020
@maryamariyan maryamariyan added this to Uncommitted in ML, Extensions, Globalization, etc, POD. via automation Oct 7, 2020
@jodydonetti
Copy link
Contributor

Hi @avin-kavish , if you still need some sort of optimized GetOrCreate for distributed caches may I suggest taking a look at FusionCache ⚡🦥 ?

It is an optionally multi-level (memory + distributed) cache with some other advanced features you may be interested in, like a fail-safe mechanism (see here) , soft/hard timeouts with background factory completion (see here) and more.

An introduction is available here.

In particular the method you are looking for is GetOrSet[Async] and is optimized for high concurrency on the same cache key (you can even provide your own impl of IDistributedCache).

I just released it and, if you find it helpful, that would be great to know.

/shameless-plug

Also, any comment on it would be greatly appreciated!

p.s.: the locking works transparently on both the local memory cache and the optional distributed cache at the same time, but I'd like to point out that what we are talking about is a local lock that will prevent multiple concurrent factory calls for the same key in the same app but not in different apps/nodes, because that would require a distributed lock and it's not something that nice to deal with.

@maryamariyan maryamariyan moved this from Uncommitted to Future in ML, Extensions, Globalization, etc, POD. Feb 3, 2021
@ericsampson
Copy link

The OP request would be useful to consider.

@peter-dolkens
Copy link

I'm here also looking for a way to achieve GetOrCreate.

I'd like a reliable way to create a distributed lock using a IDistributedCache, and whilst I'm not expecting distributed locks to be worked into IDistributedCache directly, it would be trivial to implement as an extension method if we had GetOrCreate.

e.g. an extremely limited version might be:

public Boolean AquireLock(this IDistributedCache cache, String lockID, TimeSpan lockDuration)
{
    var lockValue = $"{Guid.NewGuid()}";
    var lockResult = await cache.GetOrCreate(lockID, lockValue, lockDuration);
    return lockResult == lockValue;
}

Obviously this has serious limitations, but the intent is obvious.

As things stand now, I'm having to do Get/Set/Wait/Check loops to try and make sure a different distributed system with greater latency didn't meet a race condition setting the cache - and in our scenario, we're expecting lots of collisions, increasing the frequency of these race conditions to virtually guaranteed.

@Polymathical
Copy link

I'd like this. I think it's out of the scope of what amounts to a "helper" extensions to account for cache stampeding.

@jodydonetti
Copy link
Contributor

Look here for the new developments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Caching
Projects
None yet
Development

No branches or pull requests

9 participants