CacheTagHelper should not depend on IMemoryCache

[sebastienros]

IMemoryCache is a perfect match for single node scenarios in CacheTagHelper but it breaks if the website runs on more nodes, as each node would return different contents. The issue here is that there is no way to change it.

I think it would be better to either:

• Depend on IDitributedCache as the default implementation uses IMemoryCache which means it would work the same as today on a single instance, and not be broken on multiple instances.
• Or even better, define a custom interface for extensibility reasons so that each customer could decide how to store the cached tags: explicitly IMemoryCache, IDistributedCache or another implementation. Each site is different, and locking down the cache tag storage might be an issue. And having the to share the same implementation for controllers and tags might not make sense either.

A different approach would be to use a message bus implementation to invalidate IMemoryCache entries across nodes, but I don't think it's been planned either.

/cc @NTaylorMullen

[Eilon]

I agree with the thought in general, but I wonder if this is a case for maybe having different tag helpers? It could be argued that this tag helper is more of a <memoryCache> tag helper as opposed to just <cache> and what's described here is some other tag helper. Or not...

@DamianEdwards any further thoughts?

Parking in the Backlog for now.

[DamianEdwards]

I think the cache tag helper should have an attribute to let you select between im-memory and distributed cache.

[Eilon]

Unfortunately that breaks DI 😦 🐼

It would mean you can't take the cache interface in the ctor anymore: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.TagHelpers/CacheTagHelper.cs#L47

You'd have to reach into the service provider and resolve it.

That's the real reason I was suggesting multiple middlesware.

[sebastienros]

You can take a dependency on both.

[Eilon]

But it doesn't actually require both, so that's a bit dirty.

[sebastienros]

If Damian's advice is to have an attribute to target which storage for each tag (which is what I understood), then it actually requires both services. You can make them lazy if you want, still, not a broken design IMO.

[pranavkm]

Supporting IDistributedCache would require us to serialize the generated IHtmlContent to a byte[] which is entirely incompatible with how we cache it in IMemoryCache.
What's the use case for caching generated HTML content as opposed to caching the content that produces the HTML?

[DamianEdwards]

The case is every time I show it to people, they ask for it. StackOverflow is well known for caching rendered HTML in Redis. He user scenario is sound. Don't really care how you implement it, as long as it's in the one helper.

[Eilon]

Moving out of backlog. @sebastienros please post a proposal to discuss.

[DamianEdwards]

I don't think it's necessary for RTM. We're really beyond the time to be making changes like this.

[Eilon]

If @sebastienros can round something up would you want to take it?

[DamianEdwards]

Are you telling me that it's in effect, free? 😄

[Eilon]

Someone has to approve the design and then it needs a code review. Nothing's free 😄

[DamianEdwards]

Yeah, we have to stop some time mate. Good one for 1.1 👍

[sebastienros]

Proposal

Keep the current default behavior and store everything using the IMemoryCache service.

Add an optional distributed attribute that would use IDistributedCache. Its default value
is false. If set to true then the tag helper would resolve and use IDistributedCache to store
and retrieve the cache content.

Example:

<cache vary-by-header="Locale" distributed="true">
    ...
</cache>

If the application runs on a single instance the use of distributed won't affect the
behavior as the default IDistributedCache implementation is to use IMemoryCache.

Caveats

• The priority attribute is no-op in this case as the distributed cache API doesn't support it.
• The content is serialized to HTML.
• The Russian-doll invalidation provided by IMemoryCache is lost, meaning an inner <cache> invalidation won't invalidate its parents.
  There is still a benefit in nesting <cache> tags if the outer scope is invalidated more frequently than the inner.
• There is no default registration for this interface. We can either try to resolve
  the service and fall-back to IMemoryCache or throw an exception to make it obvious for
  the user.

[NTaylorMullen]

In the distributed cache scenario you'd need to also require the user to provide some sort of cache key so it's the same across nodes.

@sebastienros wasnt your concern with this issue initially finding a way to separate types of caches used? Aka, the cache TagHelper having a dedicated cache would enable users to separate how things are cached (db cache vs page cache vs X cache etc.).

[sebastienros]

The key is already created and unique as part as the current implementation.
Having a custom interface was one of my proposals but not an approved solution.

[sebastienros]

Actually you are right, we need a custom user key, I will look into the current implementation.

[sebastienros]

Currently it's using the TagHelperContext.UniqueKey which works on a single instance. I suggest that instead of having a distributed="true" attribute we use the key as the opt-in parameter like distributed-cache-key="mycontent". Setting this property would prove the cross instance key discriminator and also opt-in for distributed cache.

[sebastienros]

Related annoucements:
aspnet/Announcements#160
aspnet/Announcements#161