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

Added Clear method for IMemoryCache to remove all entries. #44522

Closed

Conversation

ShreyasJejurkar
Copy link
Contributor

  1. Added Clear method for IMemoryCache to remove all entries.
  2. Added corresponding test scenarios as well.

As part of #44512

public void Clear()
{
CheckDisposed();
if (!_entries.IsEmpty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race condition between IsEmpty and Clear. I'm not sure if it will be dangerous, though.
Is it possible to just call Clear (i.e. without the check for empty first)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, yes we can call directly Clear(), but I added that check just to avoid unnecessary calls to Clear().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better if the concurrent dicitonary (CD) handles this case. I mean if Clear is called and the CD is empty, it just nops. Just as thought which is not finished...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the implementation of Clear() method, I did not see any check there, everytime it's initializing new table under the hood with given size! So I think it's better to have this check here.
But am still open for any changes if required!

Copy link
Contributor Author

@ShreyasJejurkar ShreyasJejurkar Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been fixed with my other PR - #44581

@@ -103,6 +103,7 @@ public partial interface IMemoryCache : System.IDisposable
Microsoft.Extensions.Caching.Memory.ICacheEntry CreateEntry(object key);
void Remove(object key);
bool TryGetValue(object key, out object value);
void Clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MCCshreyas - thanks for the contribution!

However, since this is public API we need to follow the API review process before accepting changes in this area. See the link at the top of this file: https://aka.ms/api-review process. We won't be able to merge this PR without first getting an API approval.

An issue with the currently proposed changes is that IMemoryCache is an interface, and adding more members to an interface is a binary breaking change. This type of change isn't acceptable in these libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So need to come up with new interface for a single method? 🤔

@ghost
Copy link

ghost commented Nov 11, 2020

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


Issue meta data

Issue content: 1. Added Clear method for IMemoryCache to remove all entries. 2. Added corresponding test scenarios as well.

As part of #44512

Issue author: MCCshreyas
Assignees: -
Milestone: -

@adamsitnik adamsitnik added api-needs-work API needs work before it is approved, it is NOT ready for implementation blocked Issue/PR is blocked on something - see comments labels Dec 7, 2020
@ericstj
Copy link
Member

ericstj commented Jan 4, 2021

For new API we first need to have an approved API request before a PR is offered. I think we should close this PR for now, or convert it to draft.

@eerhardt
Copy link
Member

eerhardt commented Jan 4, 2021

Closing, per the above comment. The process outlined in https://aka.ms/api-review is to get an approved API before submitting a PR.

@eerhardt eerhardt closed this Jan 4, 2021
@ShreyasJejurkar
Copy link
Contributor Author

There is an already API in the review process => #45593.
I am closely following up with the discussion but seems like the discussion has stuck on making a decision on how to proceed with this without breaking the existing IMemoryCache interface.

If possible can we make this PR as a draft and not close ?! If the resulting implementation is going to align with this one, then I am ready to make the required changes to this PR.

Thank you!

@eerhardt
Copy link
Member

eerhardt commented Jan 5, 2021

I don’t understand the benefit of leaving it open as draft. Our process is that we don’t open PRs for new APIs until the API has been approved.

@ShreyasJejurkar
Copy link
Contributor Author

@eerhardt oh, ok! 😑

@eerhardt
Copy link
Member

eerhardt commented Jan 5, 2021

To give a little more reasoning: dotnet/runtime has 270+ open PRs right now, which is a lot to manage. We are trying to limit "unactionable" PRs from sitting and collecting dust, so people don't have to remember to ignore them.

Once we get to an approved API, we can just as easily reopen this PR to move it forward.

@ShreyasJejurkar
Copy link
Contributor Author

@eerhardt yeah. Ok. OK. No worries! 😇

Thanks for reply! 😊

@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.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Caching blocked Issue/PR is blocked on something - see comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants