-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Polly.Core Caching policy #1127
Comments
Just did a quick spike of hacking around at this locally and found the following:
I'm not sure what the best way to go about doing this natively for Polly v8 would be. Off the top of my head, it feels like we'd need a bridge interface that has sync and async methods so the strategy can just pick the right method and what happens under-the-hood is up to the implementation. Doing the above means the policy can live in the core, but by default it wouldn't actually do anything out of the box (which I think is also true of v7). We could then possibly have an additional assembly that will adapt from Using it via the extension would then be something like: // IMemoryCache
builder.AddCaching(
MemoryCache.Default,
(context, state) => /* some object */,
(cacheEntry) => cacheEntry.SlidingExpiration = TimeSpan.FromSeconds(3));
// IDistributedCache
IDistributedCache distributedCache = ...;
builder.AddCaching(
distributedCache,
(context, state) => "my-cache-key",
async (value) => await serializeToByteArray(value)); The other bit I wasn't 100% sure on was on exactly how to get the callbacks for generating the cache key and value to preserve |
Oh, and this was as far as I got with the provider just noodling around with using Microsoft.Extensions.Caching.Distributed;
using Microsoft.Extensions.Caching.Memory;
using Polly.Strategy;
namespace Polly.Caching;
internal sealed class CachingResilienceStrategy : ResilienceStrategy
{
private readonly ResilienceStrategyTelemetry _telemetry;
public CachingResilienceStrategy(
IMemoryCache cache,
ResilienceStrategyTelemetry telemetry)
{
Cache = cache;
_telemetry = telemetry;
}
public IMemoryCache Cache { get; }
public Func<OnCacheHitArguments, ValueTask>? OnCacheHit { get; }
public Func<OnCacheMissedArguments, ValueTask>? OnCacheMissed { get; }
public Func<OnCachePutArguments, ValueTask>? OnCachePut { get; }
public Func<KeyGeneratorArguments, ValueTask<object>>? KeyGenerator { get; }
protected override async ValueTask<TResult> ExecuteCoreAsync<TResult, TState>(
Func<ResilienceContext, TState, ValueTask<TResult>> callback,
ResilienceContext context,
TState state)
{
object key = await KeyGenerator(new(context, state));
if (Cache.TryGetValue(key, out TResult? result))
{
if (OnCacheHit is { } onCacheHit)
{
await onCacheHit(default).ConfigureAwait(context.ContinueOnCapturedContext);
}
return result!;
}
if (OnCacheMissed is { } onCacheMissed)
{
await onCacheMissed(default).ConfigureAwait(context.ContinueOnCapturedContext);
}
result = await callback(context, state).ConfigureAwait(context.ContinueOnCapturedContext);
var entry = Cache.CreateEntry(key);
if (OnCachePut is { } onCachePut)
{
await onCachePut(new(context, entry)).ConfigureAwait(context.ContinueOnCapturedContext);
}
return result;
}
} |
Hey @martincostello, nice job :) just a couple of thoughts about the caching:
I believe the semantics are that
My naive thinking is that we expose two strategies
I think the main concern is imposing the
I think we don't really need to propagate Regarding the API surface do you think we need these events? public Func<OnCacheHitArguments, ValueTask>? OnCacheHit { get; }
public Func<OnCacheMissedArguments, ValueTask>? OnCacheMissed { get; }
public Func<OnCachePutArguments, ValueTask>? OnCachePut { get; } I think |
Based on the recent sync, the caching strategy won't be a part of the initial V8 release. If they're is a demand from community we might induce it in a next minor release. The reason to not include:
|
Per our conversation on Thursday, I agree that we can exclude the Polly v7 caching policy from v8. If we find that excluding the caching policy impacts a lot of people (no data on whether this is the case), then we can either find a way to implement it or provide some alternatives like the upcoming set of caching libraries. |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
I was directed here by Martin Tomka after I commented on the blog post Building resilient cloud services with .NET 8. I work on a large enterprise application that is currently ASP.NET using Framework 4.8, with an effort underway to migrate to .NET 8+. We’re using the We use a Polly We knew there were trade-offs in this approach such as having to still In addition, we do have a handful of areas in our application where we use Polly’s Policy.Execute() syntax with a caching policy to cache a DTO at a higher level than the http pipeline. To further complicate things, on the clients where we’re using the To recap, if my team wants to migrate from v7 to Polly.Core, in addition to the standard stuff outlined in the migration guide we’ll need to account for:
Thanks, |
@TallBaldGeek, the caching strategy is relative simple to implement. For example: public static class CachingKey
{
public static readonly ResiliencePropertyKey<string> Id = new("Resilience.CachingKey");
}
internal class CachingStrategy(IMemoryCache cache) : ResilienceStrategy
{
protected override async ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>> callback,
ResilienceContext context,
TState state)
{
if (!context.Properties.TryGetValue(CachingKey.Id, out var key))
{
return await callback(context, state);
}
if (cache.TryGetValue<TResult>(key, out var cachedValue))
{
return Outcome.FromResult(cachedValue!);
}
var outcome = await callback(context, state);
if (outcome.Exception is null)
{
cache.Set(key, outcome.Result);
}
return outcome;
}
} And to use it: ResilienceContext context = ResilienceContextPool.Shared.Get();
context.Properties.Set(CachingKey.Id, "some-key");
resiliencePipeline.ExecuteAsync(context, c => SomeExpensiveCacheableStringOperation(), context); The strategy is based on pro-reactive implementation guidelines: So if you are blocked by this, you can do your own caching strategy in your project. If there is more Community-feedback and demand, we might introduce built-in caching strategy later. The challenge is not to duplicate and reinvent other caching API such as:
Instead, what we would want is to expose APIs that allows integrating various caches implementations easily, without us implementing our own caching layers. I am thinking of something very similar of what we did with |
If you are interested I may implement a policy based on FusionCache, which in turn can be configured to seamlessly work with both in memory and distributed caches (it has full support for both sync/async programming model, without the usual sync-over-async trick which does have its issues). Honestly it was on my todo list for some time (I mean adding FusionCache support to Polly), but first I was waiting for Polly V8 to be released and then I've been sidetracked with some other stuff recently. |
Mi thinking about cache is to introduce some low-level layer into public readonly struct CacheStoreArguments<TValue>
{
public Outcome<TValue> Outcome { get; }
public ResilienceContext Context { get; }
public string Key { get; }
}
public readonly struct CacheRetrieveArguments
{
public ResilienceContext Context { get; }
public string Key { get; }
}
public class CachingOptions<TValue> :ResilienceStrategyOptions
{
[Required]
public Func<CacheRetrieveArguments, ValueTask<Outcome<TValue>?>>? Retrieve { get; set; }
[Required]
public Func<CacheStoreArguments<TValue>, ValueTask<bool>>? Store { get; set; }
}
public static class CacheResiliencePipelineBuilderExtensions
{
public static ResiliencePipelineBuilder AddCache<TValue>(
this ResiliencePipelineBuilder<TValue> builder,
CachingOptions<TValue> options)
{
throw new NotImplementedException();
}
} The strategy will use the delegates to retrieve and store items into cache. It will also reports hit misses and any cache actions. The implementation would reside in Then we can expose some convenience overloads in public static class CacheResiliencePipelineBuilderExtensions
{
public static ResiliencePipelineBuilder AddMemoryCache<TValue>(
this ResiliencePipelineBuilder<TValue> builder,
IMemoryCache memoryCache)
{
return builder.AddCaching(new()
{
Retrieve = args =>
{
if (memoryCache.TryGetValue<TValue>(args.Key, out var cachedValue))
{
return new ValueTask<Outcome<TValue>?>(Outcome.FromResult(cachedValue));
}
return ValueTask.FromResult<Outcome<TValue>?>(null!);
},
Store = args =>
{
if (args.Outcome.Exception is null)
{
memoryCache.Set(args.Key, args.Outcome.Result);
return ValueTask.FromResult(true);
}
return ValueTask.FromResult(false);
}
});
}
public static ResiliencePipelineBuilder AddDistributedCache<TValue>(
this ResiliencePipelineBuilder<TValue> builder,
IDistributedCache distributedCache)
{
return builder.AddCaching(new()
{
Retrieve = async args =>
{
if (await distributedCache.GetAsync(args.Key) is { } cachedData)
{
// deserialize and return the result
}
return null;
},
Store = async args =>
{
if (args.Outcome.Exception is null)
{
// Serialize the result
byte[] data = ... ;
await distributedCache.SetAsync(args.Key, data, args.Context.CancellationToken);
return ValueTask.FromResult(true);
}
return ValueTask.FromResult(false);
}
});
}
} This way we can reduce our coupling to particular caching technology and make it really easy to integrate their own cache into Polly. This applies to FusionCache as well, then can just expose their own Wdyt? |
Hi @martintmk , I totally agree, I was not suggesting to add a hard link to FusionCache, that would be crazy. The idea iwould be to have a caching abstraction specific for Polly with the specific requirements needed for Polly itself, so that various impl (including but of course not limited to FusionCache) can be made. If, btw, there's currently no time to create such abstraction now, another possibility would be to start without the abstraction and try to see if it would be possible to get the caching feature as an external third party package, in this case tightly coupled with FusionCache, to get the feature out. Then, if and when the common abstraction will arrive, the package would be updated to work with the common abstraction. Just an idea. Of course I would prefer the first approach, if possible. ps: I'll look at your sample code asap, will let you know. |
@martintmk & @jodydonetti - Thanks for the quick responses and for reopening this issue for discussion. Would your proposed approaches be something that would eventually make their way into the I was hoping to have both approaches available so that we could continue to do caching at either the http message level or "one level up" to operate on a DTO. |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
Hi, since this has been marked as stale and I'm on the verge of releasing v1.0 of FusionCache, I feel it can be a good time to give it a try and make an attempt at creating a first caching policy for Polly V8, at least to see how it would work out. |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
Not stale, I hope? |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
Not stale, I hope? |
No one is actively working on it, so it basically is. |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
No news about this one? |
No, otherwise there'd be some comments or linked activity. At this stage I'm very tempted to close this off as won't do. It's been over a year since the initial proposal, and the lack of activity seems to suggest that people aren't clamouring for this functionality in Polly.Core. |
Hi, since there was interest I tried start a conversation about it:
but I haven't heard back anyone, so 🤷 |
Sorry about that - for me, the best place to discuss this is here in this issue asynchronously, not synchronously in a call somewhere. I guess that's why I didn't respond (and then no one else did either regardless of their preferences for discussion). If you have a proposal on what this would entail we can take a look at it (or submit a draft PR for comments). I'm still not fully convinced on the need for us to bake in caching as a first-class Polly primitive for the v8 API, given that it would basically just be an abstraction without any real implementations (which we'd delegate to the community), but if there's enough motivating cases to need that I could be persuaded. Maybe a better first step is to try and implement your own FusionCache-based resilience policy using the extensibility. Then it's a way to do caching with Polly v8 on top of the Core API. Then if over time there's interest in other caching-related resilience implementations (in-memory, |
No worries! I said "have a chat" but I did not necessarily mean a private one, more like "let's start discussing it", but I totally get your point.
Sounds good to me, and thanks for pointing out https://www.pollydocs.org/extensibility/index.html as a starting point, that is what I was looking for.
Makes sense, but I'd like to highlight something about the "memory cache/distributed cache alternatives", for people not familiar with FusionCache: FusionCache already uses So anyway: I'll start playing with a policy based on FusionCache and will update with what I come up with. Meanwhile any suggestion/example/tutorial/blueprint or anything else to create a custom policy, it would be much appreciated. Thanks all! |
Thanks for the explanation - I'm not familiar with FusionCache, so didn't realise it was a layer on top.
I think you're actually the first person to try that I'm aware of 😄 |
And if you happen to become more familiar and have some questions, you know who to ask to 😬
Good! Will update as soon as I have something to show. |
This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made. |
Bump to keep it open? |
I'd definitely like to work on this if a PR would be accepted? |
Just FYI, for anyone interested since I wanted to tackle the caching policy: currently I'm on this monumental beast. When I'll be finished with it, and so |
Investigate implementing a new caching policy for Polly v8 that depends on Microsoft.Extensions.Caching, rather than implementing a whole cache provider ourselves.
The text was updated successfully, but these errors were encountered: