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

Cache entry must specify a value for Size #148

Closed
onionhammer opened this issue Sep 22, 2020 · 14 comments
Closed

Cache entry must specify a value for Size #148

onionhammer opened this issue Sep 22, 2020 · 14 comments
Labels

Comments

@onionhammer
Copy link

onionhammer commented Sep 22, 2020

This should be a fairly easy one to knock out, but I'm not seeing a way to set the 'Size()' of an item for the MemoryCacheRateLimitCounterStore and other MemoryCache* providers, for when memory cache's SizeLimit is set.

 System.InvalidOperationException: Cache entry must specify a value for Size when SizeLimit is set.
   at Microsoft.Extensions.Caching.Memory.MemoryCache.SetEntry(CacheEntry entry)
   at Microsoft.Extensions.Caching.Memory.CacheEntry.Dispose()

I think "Size = 1" can be added here:

@cristipufu
Copy link
Collaborator

Other libraries have this issue as well, I don't think we can ever fix this: MiniProfiler/dotnet#501

@onionhammer
Copy link
Author

@cristipufu certainly options can be added to workaround it though

@thomasgalliker
Copy link
Contributor

I'm having this issue too. What is the proposed workaround here?

@mirdaki
Copy link

mirdaki commented Sep 28, 2020

Just ran into this issue too. @thomasgalliker Right now I'm just removing the size limit and setting shorter expiration times for my service's items. That way the part of the cache I directly control shouldn't get too big, though there will be some trial and error to set it properly.

I agree with @onionhammer that having this as an optional setting would be a nice workaround. It is unfortunate the default MemoryCache isn't more flexible.

@thomasgalliker
Copy link
Contributor

Setting the SizeLimit to null services.AddMemoryCache(o => o.SizeLimit = null); does not help in my case. How did you achieve this?

@mirdaki
Copy link

mirdaki commented Sep 28, 2020

I just don't set it at all there: services.AddMemoryCache().

@thomasgalliker
Copy link
Contributor

thomasgalliker commented Sep 28, 2020

That's now really weird. It works well with services.AddMemoryCache() when I run the asp.net core website in debug mode. As soon as I run some unit tests, the calls fail with InvalidOperationException "Cache entry must specify a value for Size". It looks like Microsoft.AspNetCore.TestHost.TestServer also uses AddMemoryCache() and sets the SizeLimit to a particular value.

When I implement my own MemoryCacheRateLimitCounterStore and replace the MemoryCacheEntryOptions object, the rate limiting also works when run from unit tests:

var options = new MemoryCacheEntryOptions { Priority = CacheItemPriority.NeverRemove, Size = 1 };

I'm not sure now who's the culprit in this game...

@onionhammer
Copy link
Author

onionhammer commented Sep 28, 2020

If you're using EF Core, you'll need to add memory cache with sizelimit of null before initializing EF Core or another dependency that initializes memorycache

This issue should be reopened IMO.

@thomasgalliker
Copy link
Contributor

Thanks for your help guys. I'll definitely give it another try tomorrow.
What about having this issue documented? I'm also thinking about promoting an own, isolated singleton instance of IMemoryCache to the rate limit store; one that is completely indepent of the other IMemoryCache instance.

@thomasgalliker
Copy link
Contributor

What about this: In ConfigureServices I no longer call AddMemoryCache() but I register singleton services for CustomMemoryCacheIpPolicyStore and CustomMemoryCacheRateLimitCounterStore. Those two Custom* services create their own instance of IMemoryCache - so, they no longer depend on MemoryCacheOptions made by other consumers of IMemoryCache (just like EFCore).

public void ConfigureServices(IServiceCollection services) {
  services.AddOptions();
  services.Configure<IpRateLimitOptions>(this.Configuration.GetSection("IpRateLimiting"));
  services.AddSingleton<IIpPolicyStore, CustomMemoryCacheIpPolicyStore>();
  services.AddSingleton<IRateLimitCounterStore, CustomMemoryCacheRateLimitCounterStore>();
  services.AddSingleton<IRateLimitConfiguration, RateLimitConfiguration>();
  services.AddHttpContextAccessor();

CustomMemoryCacheRateLimitCounterStore inherits MemoryCacheRateLimitCounterStore but instead of injecting IMemoryCache, we create an instance of IMemoryCache using MemoryCacheHelper.CreateMemoryCache(...).

public class CustomMemoryCacheRateLimitCounterStore : MemoryCacheRateLimitCounterStore
{
    public CustomMemoryCacheRateLimitCounterStore(ILoggerFactory loggerFactory)
        : base(MemoryCacheHelper.CreateMemoryCache(loggerFactory))
    {
    }
}

Same goes for CustomMemoryCacheIpPolicyStore:

public class CustomMemoryCacheIpPolicyStore : MemoryCacheIpPolicyStore
{
    public CustomMemoryCacheIpPolicyStore(
        ILoggerFactory loggerFactory,
        IOptions<IpRateLimitOptions> options = null,
        IOptions<IpRateLimitPolicies> policies = null)
        : base(MemoryCacheHelper.CreateMemoryCache(loggerFactory), options, policies)
    {
    }
}

And here we have the implementation of MemoryCacheHelper. MemoryCacheOptions uses a SizeLimit = null.

public static class MemoryCacheHelper
{
    public static IMemoryCache CreateMemoryCache(ILoggerFactory loggerFactory)
    {
        var options = new MemoryCacheOptions
        {
            SizeLimit = null
        };
        var optionsAccessor = Options.Create(options);
        return new MemoryCache(optionsAccessor, loggerFactory);
    }
}

I kindly ask for your opinion. Is this a valid solution or a cheap hack? Is there another way to achieve the same goal (having an independent IMemoryCache instance)?

@mirdaki
Copy link

mirdaki commented Oct 1, 2020

@thomasgalliker I think your solution is a reasonable workaround! I will note that it looks like you're creating two separate instances of the memory cache, which might cause some issues. I would suggest updating your static class to store the cache ones it's created, then just return the stored cache instead of creating new ones each time.

@onionhammer
Copy link
Author

I think the best solution here is also a fairly easy one - Use an internal serviceprovider (like EF Core does by default).

@cristipufu
Copy link
Collaborator

@onionhammer so basically you're suggesting that AspNetCoreRateLimit should use a new instance of MemoryCache (instead of the default one)?

@onionhammer
Copy link
Author

@cristipufu yes, exactly; a separate MemoryCache (stored in a separate ServiceProvider instance) without depending on the end-developer to set up a memorycache without a SizeLimit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants