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

Add .Clear() method to MemoryCache #45593

Closed
adamsitnik opened this issue Dec 4, 2020 · 18 comments · Fixed by #57631
Closed

Add .Clear() method to MemoryCache #45593

adamsitnik opened this issue Dec 4, 2020 · 18 comments · Fixed by #57631
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Caching
Milestone

Comments

@adamsitnik
Copy link
Member

Background and Motivation

MemoryCache gives the possibility to add and remove a single cache entry, but it does not allow for clearing the entire cache at once.

Many (more than twenty by looking at the number of reactions) of our customers have expressed the need of this API in #36547.

So far most of them was forced to use ugly workarounds like using reflection or trimming the cache to a very small size.

Proposed API

namespace Microsoft.Extensions.Caching.Memory
{
    public class MemoryCache
    {
+        public void Clear();
    }
    
    public interface IMemoryCache
    {
+        void Clear();
    }
}

Usage Examples

var cache = new MemoryCache(new MemoryCacheOptions());

cache.Set("key1", "value1");
cache.Set("key2", "value2");

cache.Clear();

Risks

Adding a new method to the IMemoryCache interface is going to break users who implemented their own cache. This should be rare.

According to my understanding, adding it with a default empty implementation would require changing the target framework from .NET Standard 2.0 to .NET 6.0?

@adamsitnik adamsitnik added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Caching labels Dec 4, 2020
@adamsitnik adamsitnik added this to the 6.0.0 milestone Dec 4, 2020
@adamsitnik adamsitnik self-assigned this Dec 4, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 4, 2020
@ghost
Copy link

ghost commented Dec 4, 2020

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

MemoryCache gives the possibility to add and remove a single cache entry, but it does not allow for clearing the entire cache at once.

Many (more than twenty by looking at the number of reactions) of our customers have expressed the need of this API in #36547.

So far most of them was forced to use ugly workarounds like using reflection or trimming the cache to a very small size.

Proposed API

namespace Microsoft.Extensions.Caching.Memory
{
    public class MemoryCache
    {
+        public void Clear();
    }
    
    public interface IMemoryCache
    {
+        void Clear();
    }
}

Usage Examples

var cache = new MemoryCache(new MemoryCacheOptions());

cache.Set("key1", "value1");
cache.Set("key2", "value2");

cache.Clear();

Risks

Adding a new method to the IMemoryCache interface is going to break users who implemented their own cache. This should be rare.

According to my understanding, adding it with a default empty implementation would require changing the target framework from .NET Standard 2.0 to .NET 6.0?

Author: adamsitnik
Assignees: adamsitnik
Labels:

api-suggestion, area-Extensions-Caching

Milestone: 6.0.0

@Joe4evr
Copy link
Contributor

Joe4evr commented Dec 4, 2020

According to my understanding, adding it with a default empty implementation would require changing the target framework from .NET Standard 2.0 to .NET 6.0?

Close; default interface implementations requires targeting .NET Core 3.0+, which removes the ability to run on full-framework or Mono.

@eerhardt
Copy link
Member

eerhardt commented Dec 4, 2020

I wonder if we could/should start moving away from the interfaces. One way of doing that would be:

  1. Introduce a new class in the Abstractions:
public abstract class MemoryCacheBase : IMemoryCache
{
    // implement all IMemoryCache methods with `abstract` methods

    public abstract void Clear();
}
  1. The current MemoryCache implementation class inherits from MemoryCacheBase and overrides and implements all the methods.

  2. In apps that use IMemoryCache want to use any new methods we define, they can change usages from IMemoryCache => MemoryCacheBase. This gives them access to the new methods, but doesn't tie them to the MemoryCache implementation.

Thoughts? cc @Tratcher @maryamariyan @davidfowl

@Tratcher
Copy link
Member

Tratcher commented Dec 4, 2020

Adding abstract methods to a base class is just as breaking for implementers as adding new methods to an interface. You can only do it once when introducing the new base class that nobody has derived from yet. The new methods would have to be virtual with a default implementation, not abstract.

There are a lot of API requests for MemoryCache, where any one of them doesn't seem significant enough to warrant a new design, but collectively they might. Is there a comprehensive list anywhere?

@eerhardt
Copy link
Member

eerhardt commented Dec 4, 2020

Is there a comprehensive list anywhere?

https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aarea-Extensions-Caching+label%3Aapi-suggestion

In general:

  • Clear
  • Enumerating the cache (which most of the scenarios I've seen is for bulk removing entries)
  • Tagging

@ShreyasJejurkar
Copy link
Contributor

Wanted to link here - #44522

@adamsitnik
Copy link
Member Author

I wonder if we could/should start moving away from the interfaces.

Personally, I don't like introducing interfaces that typically have a single implementation... So perhaps the first step to rely less on interfaces would be to just add .Clear method to MemoryCache and not include this in IMermoryCache?

@ShreyasJejurkar
Copy link
Contributor

@adamsitnik if that done so, then IMemoryCache will be redundant and it will be of no use! Because we are now started adding feature directly to MemoryCache! And somehow it might even break existing stuff dependent upon this interface as well!

@adamsitnik
Copy link
Member Author

then IMemoryCache will be redundant and it will be of no use

What is the actual (not hypothetical) value in having this particular abstraction?

The MemoryCache and CacheEntry design is not perfect (please see #36556 for some details). I would expect that the users for whom MemoryCache was not enough, have implemented their own memory cache without using the IMemoryCache abstraction (which is simply limiting).

How can we add new features to MemoryCache (we definitely need them) without introducing breaking changes to IMemoryCache and having it still target .NET Standard 2.0?

As I wrote, it's my personal opinion and I refer to @eerhardt and @maryamariyan to choose the best strategy for caching extensions.

@eerhardt
Copy link
Member

eerhardt commented Dec 7, 2020

What is the actual (not hypothetical) value in having this particular abstraction?

One big one is enabling unit testing. You can easily mock out the caching behavior.

Since pieces of ASP.NET depend on Caching, having this abstraction allows for other caching implementations to be injected into the application, and ASP.NET would start using that implementation. I don't know if this is ever done in practice.

Another piece of value is to limit dependencies. Microsoft.Extensions.Caching.Memory.csproj has dependencies on DI, Logging, Options, and Primitives. While Caching.Abstractions only depends on Primitives. So if someone didn't want to bring in DI, Logging, and Options, they could potentially write their own cache implementation without these dependencies.

@adamsitnik
Copy link
Member Author

One big one is enabling unit testing.

@eerhardt you have convinced me, thank you!

@shaynevanasperen
Copy link

I've just solved this problem for myself (I needed to reset the cache between running each scenario of my unit tests), like this:

public sealed class MemoryCacheManager : IMemoryCache
{
    readonly IOptions<MemoryCacheOptions> _optionsAccessor;
    
    IMemoryCache _inner;

    public MemoryCacheManager(IOptions<MemoryCacheOptions> optionsAccessor)
    {
        _optionsAccessor = optionsAccessor;
        _inner = new MemoryCache(_optionsAccessor);
    }

    public void Reset()
    {
        var old = _inner;
        _inner = new MemoryCache(_optionsAccessor);
        old.Dispose();
    }

    public void Dispose() => _inner.Dispose();

    public ICacheEntry CreateEntry(object key) => _inner.CreateEntry(key);

    public void Remove(object key) => _inner.Remove(key);

    public bool TryGetValue(object key, out object value) => _inner.TryGetValue(key, out value);
}

And then in app startup:

services.Remove(services.Single(x => x.ServiceType == typeof(IMemoryCache)));
services.AddSingleton<MemoryCacheManager>();
services.AddSingleton<IMemoryCache>(provider => provider.GetRequiredService<MemoryCacheManager>());

Then I can just resolve an instance of MemoryCacheManager from IoC and call Reset() when I want to clear the cache.

@maryamariyan maryamariyan removed this from the 6.0.0 milestone Feb 3, 2021
@maryamariyan maryamariyan added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 4, 2021
@maryamariyan maryamariyan added this to the Future milestone Mar 4, 2021
@bartonjs
Copy link
Member

bartonjs commented Aug 17, 2021

Video

  • The interface can't be changed, per our breaking change rules.
  • The method also can't be added as a DIM, because the library targets .NET Standard 2.0
  • The primary method is OK.
  • Perhaps it's time to mark the interface as [Obsolete]? Something to think about.
namespace Microsoft.Extensions.Caching.Memory
{
    partial class MemoryCache
    {
        public void Clear();
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 17, 2021
@Tratcher
Copy link
Member

Can the DIM be added only for compatible targets?

@eerhardt
Copy link
Member

Can the DIM be added only for compatible targets?

How would that work?

@Tratcher
Copy link
Member

Tratcher commented Aug 17, 2021

e.g. Cross target the package to netstandard and net5.0 and only add the DIMs for net5.0.

@eerhardt
Copy link
Member

And what would the DIM do? throw?

@Tratcher
Copy link
Member

And what would the DIM do? throw?

If not overridden, yes.

@adamsitnik adamsitnik modified the milestones: Future, 7.0.0 Aug 18, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 18, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Caching
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants