Support for "cache-control:no-cache" in request #196
Conversation
abpatel
commented
Jan 28, 2017
abpatel
commented
- Added Support for "cache-control:no-cache" in request
- Updated CacheOutputUntil in client side unit test
- Updated NUnit version and added test runner
2)Updated CacheOutputUntil in client side unit test 3) Updated NUnit version and added test runner
| @@ -223,6 +231,11 @@ public override void OnActionExecuting(HttpActionContext actionContext) | |||
| var responseMediaType = actionExecutedContext.Request.Properties[CurrentRequestMediaType] as MediaTypeHeaderValue ?? GetExpectedMediaType(httpConfig, actionExecutedContext.ActionContext); | |||
| var cachekey = cacheKeyGenerator.MakeCacheKey(actionExecutedContext.ActionContext, responseMediaType, ExcludeQueryStringFromCacheKey); | |||
|
|
|||
| if (IsNoCacheHeaderInRequest(actionExecutedContext.ActionContext)) | |||
There was a problem hiding this comment.
this shouldn't be here - once a no cache request has been sent by any client, it shouldn't invalidate cache for all clients
|
thanks, can you please incorporate the requested changes? |
| @@ -242,12 +255,12 @@ public override void OnActionExecuting(HttpActionContext actionContext) | |||
| _webApiCache.Add(baseKey, string.Empty, cacheTime.AbsoluteExpiration); | |||
| _webApiCache.Add(cachekey, content, cacheTime.AbsoluteExpiration, baseKey); | |||
There was a problem hiding this comment.
Wouldn't this effectively invalidate the cache for all clients anyways since we are adding(replacing) the cache entry for the same exact cache key that we are explicitly removing in Line 234 ? Since the cache is public and not a per client private cache, do you see any use cases wherein it's harmful to have the cache invalidated for a specific key "globally", if requested so by one client by specifying "cache-control:no-cache"
There was a problem hiding this comment.
so this code would only run if the request wasn't cached in the first place.
I don't think you should invalidate the cache for everyone by any client call, since that would defeat the purpose of caching at all. I could then write a malicious client that just busts all the cache on the server all the time, which is definitely not good
There was a problem hiding this comment.
Wouldn't it also run if we are bypassing the cache in 'OnActionExecuting' by checking if the client requested 'no-cache'?
Should we then not cache the fresh response at all for that, specific client call and just return the data we retrieve from executing the requested action and let the existing (stale) cached response be as is, so that it eventually expires per the expiration policy that was set on the action?
There was a problem hiding this comment.
Yeah, I do understand your point, since we already have the response at this point, we might as well use it to update the cache.
My main concern is that, in reality, cache-control headers are not really intended for the server, they are really intended for clients + intermediaries (such as proxies). So they instruct clients how to handle responses in terms of caching, and they instruct proxies how to deal with requests.
Building a convoluted caching logic on the server side based on them is not necessarily a good idea, or I am not sure everyone would be happy to have their cache getting busted at any point by and client from anywhere in the world. If you rely on caching to do performance lift for you, this is a performance black hole.
Maybe a better idea is to have this feature controlled by a flag on the caching attribute, something like [CacheOutput(AllowCacheBypass = true)]
There was a problem hiding this comment.
I agree with all your points. When I read the http spec on 'no-cache', it was light on details on whether it is specific to client side only. Although majority of cases are for client side use, there is nothing in the spec that precludes using it on the server.
Perhaps the easiest option then, is to bypass the cache completely and let it expire on it own per the expiration period. I think the additional attribute makes sense and we can use that in combination with the presence or absence of no-cache.sound good?
There was a problem hiding this comment.
One other thought is to use something like
[CacheOutput(AllowPrivate=true, AllowCacheBypass=true)] which enables a per client cache if the client passes in a 'cache-control: private' header, we could then allow for using 'no-cache' if the client chooses to refresh their cache, we then invalidate their private cache, invoke the action, cache the new data (as private again) and return back. This way the client cannot affect any other client and a malicious client would constantly be invalidating only their cache at the expense of the server spinning cpu cycles to invalidate. Thoughts?
There was a problem hiding this comment.
Sorry to jump into this discussion without invitation, but I thought on something similar: when a client issues a no-cache is actually saying "bypass whatever you have cached", but not saying that "you should remove your cached data".
I implemented something similar in a project and our approach was much simpler: if the client sent a no-cache it was exactly the same as if the controller said the results shouldn't be cached. And to implement so we overloaded the IsCachingAllowed, adding a condition similar to what's evaluated in @abpatel IsNoCacheHeaderInRequest.
If you like, I can create a PR with that.
There was a problem hiding this comment.
The more the merrier. :-)... That's something I've considered as well, but the issue IMHO that you run into is that at what point would the client know NOT to use no-cache anymore? I imagine the client would have to do some sort of time calculation based on max-age to know when the old cached value expires so that it can stop sending no-cache, and reap the benefits of caching instead of passing through the cache everytime
|
Any update on this? I'm pretty sure this is causing 502 errors for my clients calling my service through a proxy. |
|
I'm working through the PR for this with Filip. There are a couple of things we are working on sorting out. Once we work through this, hopefully soon, we can have the PR merged. Thanks. Abhijeet On Mar 16, 2017 9:38 AM, Kilowhisky <[email protected]> wrote:Any update on this? I'm pretty sure this is causing 502 errors for my clients calling my service through a proxy.
—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Just to keep things ordered, I assume this PR tackles #190 |
2) Add unit test to ensure that no cache conrol / etag headers are sent in respone when no-cache sent in request
|
@filipw For now, I've removed the cache invalidation when no-cache is present in the request headers. |
|
Update? |
|
@filipw can you merge the PR for this please? |
| @@ -175,6 +181,8 @@ public override void OnActionExecuting(HttpActionContext actionContext) | |||
|
|
|||
| if (!_webApiCache.Contains(cachekey)) return; | |||
|
|
|||
| if (IsNoCacheHeaderInRequest(actionContext)) return; | |||
There was a problem hiding this comment.
can you move this up? there is no need to go through cache key generator if this is the case.
I'd put it right after IsCachingAllowed()
| @@ -223,6 +234,11 @@ public override void OnActionExecuting(HttpActionContext actionContext) | |||
| var responseMediaType = actionExecutedContext.Request.Properties[CurrentRequestMediaType] as MediaTypeHeaderValue ?? GetExpectedMediaType(httpConfig, actionExecutedContext.ActionContext); | |||
| var cachekey = cacheKeyGenerator.MakeCacheKey(actionExecutedContext.ActionContext, responseMediaType, ExcludeQueryStringFromCacheKey); | |||
|
|
|||
| //if (IsNoCacheHeaderInRequest(actionExecutedContext.ActionContext)) | |||
There was a problem hiding this comment.
can you please remove the commented code?
|
@abpatel sorry for the delay - I just left two small comments, can you address them please |
|
Not sure how to address these code changes on this pull request so i made another one #207 |
