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

Revisit MemoryCache design #36556

Closed
JunTaoLuo opened this issue Mar 28, 2017 · 6 comments
Closed

Revisit MemoryCache design #36556

JunTaoLuo opened this issue Mar 28, 2017 · 6 comments

Comments

@JunTaoLuo
Copy link
Contributor

The current design of MemoryCache is awkward due to the IMemoryCache interface where ICacheEntry CreateEntry(object key) exists instead of a more intuitive Set(object key, object value). The expected usage pattern is to call Dispose() on the ICacheEntry to "commit" the entry to the cache but this is unintuitive as well. This proved to be an issue when attempting to implement a strategy based eviction trigger and strategy in aspnet/Caching#280. We should revisit this design and see if there are possible improvements.

cc @davidfowl @DamianEdwards

@muratg
Copy link

muratg commented Jan 3, 2018

Putting this tentatively in 2.2.0.

@muratg
Copy link

muratg commented Aug 31, 2018

This is not currently in scope. We'll revisit in a future release.

cc @DamianEdwards

@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
Copy link
Contributor

The fact that you have to CreateEntry and then Dispose the returned value to add it to the cache has been very non-intuitive to users. It'd be nice to take a look at a better API here.

@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
Copy link
Member

@JunTaoLuo is this still needed? (Referring to aspnet/Caching#280 (comment))

Also since we have a Set extension method here:

public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value)
{
using ICacheEntry entry = cache.CreateEntry(key);
entry.Value = value;
return value;
}

@JunTaoLuo
Copy link
Contributor Author

The point of the issue is that the pattern of using CreateEntry and then calling Dispose to add the entry to the cache is unintuitive. We do have more convenient helpers like Set so that most people don't need to directly use that pattern when interacting with a cache. However, the underlying issue of the unintuitive API remains. That said, given that there hasn't been much traffic on this issue, maybe it's not a problem for most users, in which case we can either backlog or close this issue.

@adamsitnik
Copy link
Member

The current design is far from perfect. I agree that we need a re-design. Pplease follow #48567 which includes your proposal and some other good ideas. Thank you!

@adamsitnik adamsitnik closed this as not planned Won't fix, can't repro, duplicate, stale Nov 15, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants