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

Will there be GetEnumerator() for MemoryCache ? #36554

Closed
ardpx opened this issue Feb 21, 2016 · 24 comments · Fixed by #94992
Closed

Will there be GetEnumerator() for MemoryCache ? #36554

ardpx opened this issue Feb 21, 2016 · 24 comments · Fixed by #94992
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Caching
Milestone

Comments

@ardpx
Copy link

ardpx commented Feb 21, 2016

LINK to the proposal: #36554 (comment)

Just wonder if there would be GetEnumerator() for MemoryCache, as supported in System.Runtime.Caching.Memorycache?

@Eilon
Copy link
Member

Eilon commented Feb 21, 2016

No current plans for it. The idea is that the moment after you enumerate - or even while you enumerate, the cache may have purged some of the entries.

Can you share more about why you want this?

BTW we are looking at adding more diagnostic-related info to the cache system.

@ardpx
Copy link
Author

ardpx commented Feb 21, 2016

I am trying to build the cache like replica of the db table, so that it avoid frequent db hit in my project of device farm monitoring and management. And there's also one API which needs to read all the device info from the cache.
It's okay in my case to read the stale data even if it's polluted right after getAll(), as I just need a snapshot of the farm status at some time.

@Eilon
Copy link
Member

Eilon commented Feb 21, 2016

Have a look at https://github.com/aspnet/Caching/issues/129. Would that solve some of your needs?

@ardpx
Copy link
Author

ardpx commented Feb 21, 2016

Seems not really, as I was looking for using cache as replica of authoritative master and control when to write the cache to DB. So I need 'select *' from cache as from db table.

@muratg
Copy link

muratg commented Feb 29, 2016

@ardpx If you have a single entry point where you insert data, you can populate a list of keys yourself, right? Making this general, is kind of dangerous for the issues Eilon outlined above.

@sebastienros
Copy link
Member

This class is not supposed to provide GetEnumerator and I also opened aspnet/Caching#162 which is somehow related.

@lodejard
Copy link

Do you mean you're storing the updated data in cache, and later sending the updates to the database? A problem you will have there is the cache can remove them before you commit the changes. If you're using the cache to reduce "read" operations that's a perfectly fine idea, but if you're looking to have background or batched "write" operations you should consider creating a class to queue up the work items which will be committed later. You can so that with a singleton service that holds a list of work item classes.

@Luca7993
Copy link

Luca7993 commented Apr 1, 2016

I would need the getenumerator too.
In my case I need to retrieve all items that starts with a particular string constant.
For the moment, I keep a list of keys added to the cache as suggested, but I think it's better to have enumerator and retrieve the Keys without using an external list

@Grauenwolf
Copy link

There are many times when I would love the be able to read (or delete) all of the cache entries based on a prefix. For example, I sometimes want to clear every cache entry containing a Product object because a user just changed our discount rates.

Ideally I would write .ClearByPrefix("Product: ").

Currently I just clear the whole cache, which of course removes unrelated items.

@fidou
Copy link

fidou commented Oct 16, 2018

The ticket is bit old but still relevant in my case.
it would be nice to remove several keys from the IDistributedCache based on a regular expression. Enumeration is not necessary in my case (although it would be handy sometimes)
Is there any progress on this issue?

@aspnet-hello aspnet-hello transferred this issue from aspnet/Caching Dec 13, 2018
@analogrelay analogrelay transferred this issue from dotnet/extensions May 15, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Caching untriaged New issue has not been triaged by the area owner labels May 15, 2020
@analogrelay analogrelay added this to the Future milestone May 15, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@maryamariyan maryamariyan added feature-request api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed feature-request labels Sep 30, 2020
@maryamariyan maryamariyan added this to Triaged in ML, Extensions, Globalization, etc, POD. via automation Sep 30, 2020
@maryamariyan maryamariyan moved this from Uncommitted to Future in ML, Extensions, Globalization, etc, POD. Feb 3, 2021
@Naidile-P-N
Copy link

Naidile-P-N commented Jul 1, 2021

I want to monitor the memory usage, space consumed by each key in the cache created within the application.
Is there a way to evaluate this?

@patchu
Copy link

patchu commented Jul 5, 2021

I want to echo similar comments above. I'm migrating from the old HTTPContext cache which did have an enumerator in it. I have sets of keys that all start with a specific string that I want to delete. Yes, I COULD keep a list of keys in my own array, but then I'd be writing additional code to maintain information that already exists in the cache. For Javascript objects, this is simply cache.keys(). Why not something simple like that?

@kwacks
Copy link

kwacks commented Apr 18, 2023

Not sure why this basic scenario isn't supported - most modern cache providers support enumerating keys - one core scenario like many others pointed out is the ability to delete keys with a certain prefix. Necessary if you're maintaining IMemoryCache as a singleton and want to avoid deleting all items in the cache at once.
Is there any plan to support this?

@udlose
Copy link

udlose commented May 4, 2023

Tracking cache keys myself is at best a hacky and dangerous solution. If the StackExchange.Redis implementation can do this for a Distributed Cache (which it does quite well), there is no reason you couldn't take a read-only snapshot and return that to the caller. Of course, the caveat is known that the list could change after the snapshot is taken but I'd think that should be obvious to any consumer..

@adamsitnik
Copy link
Member

I was performing an audit of Microsoft.Extensions.Caching.Memory today and realized that this is one of the most up-voted feature requests.

First of all, same as for Clear we can't extend IMemoryCache as it would be a breaking change (#45593 (comment)). So our focus should be on the concrete implementation: MemoryCache.

MemoryCache is more or less a wrapper around ConcurrentDictionary, so:

  • implementing the enumeration should be simple (fad4c91).
  • implementing a performant and thread-safe bulk remove based on a condition is not possible as ConcurrentDictionary itself does not support it. I would prefer not to expose a non-performant version, as it would be performance pit of failure the the end users.

When it comes to the enumeration, we have two options: make MemoryCache implement IEnumerable or implement GetEnumerator that returns struct that implements the pattern (this is a duck typing recognized by C# compiler).

The disadvantage of the first approach is the clutter IDE gives me for everything that implements IEnumerable:

image

The disadvantage of the latter approach is lack of good discoverability.

@bartonjs @stephentoub Which approach would you recommend?

@stephentoub
Copy link
Member

The disadvantage of the first approach is the clutter IDE gives me for everything that implements IEnumerable

Those glyphs at the bottom of the IntelliSense popup allow you to filter out extension methods if you don't want to see them.

The disadvantage of the latter approach is lack of good discoverability.

The bigger disadvantage is it means you can't use LINQ (or, at least, to use LINQ you need to play tricks like writing your own iterator and then using LINQ over that). I assume that's something folks would like to be able to do.

Which approach would you recommend?

If we want to address this issue, I'd want to see it implement the interface. It could do so explicitly rather than implicitly and then you could also have a GetEnumerator that returns a struct if there were allocation concerns; then foreach'ing over it directly wouldn't allocate but you'd still be able to use extension methods on IEnumerable, and they'd use whatever explicit GetEnumerator implementation was provided with whatever object it wants to return.

@bartonjs
Copy link
Member

I can't think of any places where we've done (2) without also doing (1), off the top of my head.

One way to get fairly close is to add something like public IEnumerable<???> EnumerateItems(). So it'll enable cache.EnumerateItems().Where(entry.Key.StartsWith(somePrefix)).[...] without "polluting" the object itself. Typing more words might also convey "this isn't necessarily the best of approaches", vs collection types where enumeration is more expected.

@adamsitnik
Copy link
Member

adamsitnik commented Nov 20, 2023

Background and Motivation

MemoryCache gives the possibility to add and remove values for given keys, but it does not expose a possibility to read keys added to the cache.

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

So far most of them were forced to use ugly workarounds like creating a wrapper around MemoryCache that would be also storing their own collection of keys added to the cache.

Proposed API

Expose IEnumerable<object> Keys, with a different behavior than ConcurrentDictionary.Keys: don't perform a snapshot, use lazy-evaluation instead (perf).

namespace Microsoft.Extensions.Caching.Memory
{
    public class MemoryCache
    {
+       public IEnumerable<object> Keys { get; }
    }

Usage Examples

MemoryCache cache = new(new MemoryCacheOptions());

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

foreach (object key in cache.Keys)
{
    if (key is string text && text.StartsWith("key"))
    {
        cache.Remove(key);
    }
}

Alternative Designs

Extending MemoryCache with IEnumerable<KeyValuePair<object, ICacheEntry>> implementation was considered, but:

image

Risks

This proposal avoids a risk of breaking change, by not extending the IMemoryCache interface.

However, our users may not understand the concept of a snapshot and:

  • assume that the content of Keys read once from MemoryCache is always in sync with the cache.
  • assume that no copy is being made until the Keys are enumerated for the first time.

But this has not stopped us from adding this API to ConcurrentDictionary and I hope it's also not going to stop us this time.

@adamsitnik adamsitnik added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 20, 2023
@adamsitnik adamsitnik self-assigned this Nov 20, 2023
@adamsitnik adamsitnik modified the milestones: Future, 9.0.0 Nov 20, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 20, 2023
@stephentoub
Copy link
Member

stephentoub commented Nov 20, 2023

But this has not stopped us from adding this API to ConcurrentDictionary and I hope it's also not going to stop us this time.

ConcurrentDictionary implements IDictionary, so it had to have Keys (at least explicitly).

And from my perspective we made a mistake giving it the semantics we did. Didn't you spend a bunch of time reducing overheads in MemoryCache? And now we'd be implementing a property with O(N) allocation on every property read?

@adamsitnik
Copy link
Member

@stephentoub What other options do we have considering that:

  • users want to iterate over it to find keys that they want to remove (having a copy helps here)
  • MemoryCache is a wrapper around ConcurrentDictionary and it can use only public method/properties

@stephentoub
Copy link
Member

What other options do we have

public IEnumerable<object> GetKeys()
{
    foreach (KeyValuePair<object, CacheEntry> pairs in _dictionary)
    {
        yield return pairs.Key;
    }
}

?

@adamsitnik
Copy link
Member

@stephentoub I've given it a try in 4275180 and updated the proposal (I was unaware of the fact it's possible to enumerate over concurrent dictionary and modify it at the same time). Thanks, it was great idea!

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 22, 2023
@bartonjs
Copy link
Member

bartonjs commented Jan 9, 2024

Video

  • During review there was a question of whether it should be IEnumerable<object> or should use a custom struct-based enumerator-pattern type; but the answer to that depends on usage data we don't have, so it's a question for the area owners.
namespace Microsoft.Extensions.Caching.Memory
{
    public partial class MemoryCache
    {
       public IEnumerable<object> Keys { get; }
    }
}

@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 Jan 9, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 16, 2024
@thompson-tomo
Copy link

@adamsitnik I am attempting to implement an actuator to provide info about the cache/clear it as per how spring boot does it (https://docs.spring.io/spring-boot/docs/current/actuator-api/htmlsingle/#caches) and i would propose we make a change to the data model being returned to provide additional metadata and this is the best time to do it before it is released. I have created a proposal #98251 to cover this for the memory cache option could you take a look.

Note we should be able to address the breaking change aspect by providing a default implementation.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2024
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.