Skip to content

Response caching middleware doesn't allow injection of IResponseCache #2957

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

Open
angrymrt opened this issue Mar 14, 2018 · 22 comments
Open

Response caching middleware doesn't allow injection of IResponseCache #2957

angrymrt opened this issue Mar 14, 2018 · 22 comments
Labels
affected-few This issue impacts only small number of customers area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-response-caching severity-minor This label is used by an internal tool
Milestone

Comments

@angrymrt
Copy link

angrymrt commented Mar 14, 2018

The response caching middleware uses a MemoryCache object to cache responses and it is hardcoded in the public constructor to use it. See: https://github.com/aspnet/ResponseCaching/blob/master/src/Microsoft.AspNetCore.ResponseCaching/ResponseCachingMiddleware.cs. This makes it impossible to provide your own implementation of IResponseCache. I would for example like to create a DistributedResponseCache so I can have better horizontal scaling.

@tpeczek
Copy link

tpeczek commented Mar 14, 2018

You can go around this, please take a look here: https://www.tpeczek.com/2018/03/redis-backed-response-caching-in-aspnet.html

@angrymrt
Copy link
Author

@tpeczek thanks, this is useful as a temporary hack, but I would prefer an official solution for this.

@tpeczek
Copy link

tpeczek commented Mar 14, 2018

@angrymrt I went arround, because according to discussion here it has been done on purpose.

@shirhatti
Copy link
Contributor

@JunTaoLuo What do think next steps here are? Is this work-around fine? Or did you already have a design in mind?

@JunTaoLuo
Copy link
Contributor

The workaround isn't ideal since it uses reflection but it does get the job done.

@angrymrt
Copy link
Author

@JunTaoLuo how about a dependency on an IResponseCacheFactory that has a Create(IOptions options) method?

Then just pass the IResponseCacheFactory into the internal constructor and use it to create a cache (either distributed or memory).

@JunTaoLuo
Copy link
Contributor

There are certainly many ways to build this functionality. I'm not sure when we'll triage this work item though.

@analogrelay analogrelay added enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed triage-review labels Jul 24, 2019
@analogrelay analogrelay added this to the Backlog milestone Jul 24, 2019
JunTaoLuo pushed a commit that referenced this issue Apr 1, 2020
…ource

Fix LoggingEventSource to handle null strings
@yringler
Copy link

yringler commented May 5, 2020

@JunTaoLuo , unfortunately that workaround no longer works, because this commit removes public access to the IResponseCache interface.
Perhaps could that be made public again?

@slangeder
Copy link

@JunTaoLuo , unfortunately that workaround no longer works, because this commit removes public access to the IResponseCache interface.
Perhaps could that be made public again?

+1 to that. I ended up copying the AspNetCore ResponseCaching code into our repository and adapted the visibility modifiers. Not proud about that, but now we can upgrade to AspNetCore 3.1.
Why can't the ResponseCaching feature make use of the IDistributedCache interface like so many other AspNetCore features?

@yringler
Copy link

yringler commented May 5, 2020

I'm poking around with maybe making a nuget package which people can use. It uses aspnetcore as a submodule.
The basic idea is to link to all the asnetcore source files through the submodule into a new project, and then add more IResponseCache implementations to that new project.
The only concern is that I end up going down a rabibit hole of linking to more and more source files... But I think it'll be ok.

update
It works! I'm sure it could be improved, but it does the job.
Available on nuget as DistributedResponseCachingMiddleware
@slangeder, does it fulfill the requirements for you're project?

@slangeder
Copy link

@yringler Thank you very much for creating that package.
A coworker included it in our project today, and it works perfectly.

@dustinmoris
Copy link

dustinmoris commented May 12, 2020

Possible duplicate or at least closely related to #2617.

Can we priorotise this work again? If I was to send in a PR what are the chances of you guys merging it in for the next release?

EDIT:

The current response caching implementatiom is extremely frustrating. It means that an app caches the same data on every node where it's been deployed, which makes it even dumber when you think that you can run more pods in Kubernetes than physical nodes, effectively the same node caching the same data multiple times and wasting valuable and costly cloud resources. Having a configurable public interface to allow an IDistributedCache would make so much more sense.

Not to mention that the more nodes have to cache a response the less likely it is that a cached response will be returned on a request, because there's a chance that each request hits a different node, effectively defeating the purpose of caching in the first place.

Not sure why this was ever approved/shipped in this form, but it needs urgent fixing!

@yringler
Copy link

yringler commented May 12, 2020

Yes, a resolution of #2617 would also resolve this. (Unless the resolution is also done with private/internal classes.) It's a particular place where the functionality requested in this issue is needed.

Feel free to take a look at my repo mentioned earlier, available on nuget.

It is a replacement for Microsoft.AspNetCore.ResponseCaching which allows customizing the response cache, with a bunch of ready made implementations, for example, one which accepts an injected IDistributedCache. It's implemented with all the official Microsoft code internally via git submodule and csproj linking, so the amount of code in it which isn't coming from Microsoft is pretty tiny.

@slangeder is using it, and I have tested it at my company, although we aren't using it in production yet.

If you see something which needs improvement, it's under the MIT license if you want to fork, or you can drop me a PR.

@JunTaoLuo JunTaoLuo added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@JunTaoLuo
Copy link
Contributor

Extensibility is something we'd want to keep in mind when designing Output Cache: #2627.

@brunolm
Copy link

brunolm commented May 4, 2021

Any updates on this? What's the current workaround?

@yringler
Copy link

yringler commented May 4, 2021

The only workaround I'm aware of is kinda to fork MS code 🤷
Even the nice guy I was working with on my proj, after he couldn't use it b/c of his company rules, said that they were going to use the same approach, namely to reference the MS code with CSPROJ links, to access all the internal classes.
There's a workaround here, which basically uses the MS code, opens it up, and adds some functionality.
I've been meaning to get around to updating the docs and republishing a nuget, but haven't yet. Feel free to take ownership of it!

@brunolm
Copy link

brunolm commented May 6, 2021

I found a way to implement a custom filter attribute that can handle server caching. This works for me on dotnet core 5.0

note: This is not fully finished, it needs to get values from params.

namespace Something
{
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
    public class ActionResultCacheAttribute : ActionFilterAttribute
    {
        private string _cacheKey;

        /// <summary>
        /// The comma separated parameters to vary the caching by.
        /// </summary>
        public string VaryByParam { get; set; }
        public string VaryByHeader { get; set; }
        public string VaryByQueryKeys { get; set; }
        public bool VaryByAuth { get; set; } = false;


        public int Duration { get; set; }
        public int SlidingDuration { get; set; }

        public override async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
        {
            var _cache = context.HttpContext.RequestServices
                .GetService(typeof(IDistributedCache)) as IDistributedCache;

            this._cacheKey = GetCacheKey(context);

            var resultStr = await _cache.GetStringAsync(this._cacheKey);

            if (resultStr != null)
            {
                var result = Newtonsoft.Json.JsonConvert.DeserializeObject<ObjectResult>(
                    resultStr
                );
                result.Value = result.Value?.ToString();
                context.Result = result;
                return;
            }

            await base.OnActionExecutionAsync(context, next);
        }

        /// <summary>
        /// Occurs when an action has executed.
        /// </summary>
        /// <param name="filterContext">The filter context.</param>
        public async override void OnActionExecuted(ActionExecutedContext filterContext)
        {
            if (filterContext.Exception != null || string.IsNullOrWhiteSpace(this._cacheKey))
            {
                return;
            }

            var _cache = filterContext.HttpContext.RequestServices
                .GetService(typeof(IDistributedCache)) as IDistributedCache;

            var result = Newtonsoft.Json.JsonConvert.SerializeObject(filterContext.Result);

            await _cache.SetStringAsync(this._cacheKey, result, new DistributedCacheEntryOptions
            {
                AbsoluteExpirationRelativeToNow = Duration > 0 ? TimeSpan.FromSeconds(Duration) : null,
                SlidingExpiration = SlidingDuration > 0 ? TimeSpan.FromSeconds(SlidingDuration) : null,
            });
        }

        private string GetCacheKey(ActionExecutingContext context)
        {
            var routeValues = context.RouteData.Values;
            var actionParameters = context.ActionArguments;
            var username = context.HttpContext?.User?.Claims?.FirstOrDefault(o => o.Type == ClaimTypes.Sid)?.Value;

            var sb = new StringBuilder(routeValues["controller"].ToString());
            sb.Append('_').Append(routeValues["action"].ToString());

            if (!string.IsNullOrWhiteSpace(VaryByParam))
            {
                sb.Append(VaryByParam);
            }

            if (!string.IsNullOrWhiteSpace(VaryByHeader))
            {
                sb.Append(VaryByHeader);
            }

            if (!string.IsNullOrWhiteSpace(VaryByQueryKeys))
            {
                sb.Append(VaryByQueryKeys);
            }

            if (VaryByAuth)
            {
                sb.Append($"U-{username}-");
            }

            return sb.ToString();
        }
    }
}

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
@Ognen67
Copy link

Ognen67 commented Sep 4, 2023

The only workaround I'm aware of is kinda to fork MS code 🤷 Even the nice guy I was working with on my proj, after he couldn't use it b/c of his company rules, said that they were going to use the same approach, namely to reference the MS code with CSPROJ links, to access all the internal classes. There's a workaround here, which basically uses the MS code, opens it up, and adds some functionality. I've been meaning to get around to updating the docs and republishing a nuget, but haven't yet. Feel free to take ownership of it!

Does this still work now with the Microsoft.AspNetCore.ResponseCaching package Version: 2.2.0 ?

@martincostello
Copy link
Member

Does this still work now with the Microsoft.AspNetCore.ResponseCaching package Version: 2.2.0 ?

That NuGet package is deprecated and doesn't receive updates.

@Ognen67
Copy link

Ognen67 commented Sep 4, 2023

Does this still work now with the Microsoft.AspNetCore.ResponseCaching package Version: 2.2.0 ?

That NuGet package is deprecated and doesn't receive updates.

Okay, thanks for the prompt response.

@Ognen67
Copy link

Ognen67 commented Sep 4, 2023

Is there another way to achieve the same results as of now?

@samsp-msft
Copy link

@mgravell - do we still need this item?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-response-caching severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests