Cache-Control: no-cache breaks the middleware #81

colltoaction · 2016-11-28T07:26:41Z

TL;DR: this breaks with the Cache-Control: no-cache header since the IResponseCachingFeature feature is not set up and we hit this exception.

Hi,

I'm just starting with your middleware, and the basic functionality is great. I just specify a [ResponseCache] and it gets cached automatically, which is awesome :).

But I'm getting an error 500 each time I do "Ctrl+F5" or I disable caching in the Chrome dev tools. The error is the one in the title:

An unhandled exception occurred while processing the request. InvalidOperationException: 'VaryByQueryKeys' requires the response cache middleware.

Microsoft.AspNetCore.Mvc.Internal.ResponseCacheFilter.OnActionExecuting(ActionExecutingContext context)

The error is the same as if I had not called app.UseResponseCaching(); or called it in the wrong order. The exact line is the following, which shows that it's checking for a Feature of type IResponseCachingFeature: https://github.com/aspnet/Mvc/blob/d8c6c4ab34e1368c1b071a01fcdcb9e8cc12e110/src/Microsoft.AspNetCore.Mvc.Core/Internal/ResponseCacheFilter.cs#L132.

I investigated the source code and came to the conclusion that the error is produced because:

Browsers send Cache-Control: no-cache header
The request is marked as not cacheable here
Since it's not cacheable we skip the call to ShimResponseStream
And we don't set up the Feature for the next middlewares.
So we end up with the mentioned exception in this line

If you could give me any workaround until a fix is published I'd really appreciate it!

Thanks,
Martín.

The text was updated successfully, but these errors were encountered:

Tratcher · 2016-11-29T00:16:05Z

@tinchou Thanks for the detailed investigation. We'll get this dealt with ASAP. One possible workaround would be to always set an IResponseCachingFeature. Try putting this between the caching middleware and MVC:

app.Use((context, next) =>
{
  if (context.Features.Get<IResponseCachingFeature>() == null)
  {
    context.Features.Set<IResponseCachingFeature>(new FakeResponseCachingFeature());
  }
  return next();
});

@muratg @DamianEdwards This is a patch candidate bug.

colltoaction · 2016-11-29T16:43:41Z

Can confirm the workaround works for me.

I implemented FakeResponseCachingFeature as:

    public class FakeResponseCachingFeature : IResponseCachingFeature
    {
        public string[] VaryByQueryKeys
        {
            get => null;
            set { }
        }
}

JunTaoLuo · 2016-11-29T21:34:07Z

@pranavkm we'll need to build a 1.1.1 branch for this.

JunTaoLuo · 2016-11-30T17:51:49Z

@Tratcher and I think the middleware needs to be fixed. The feature should be added regardless of whether the request is cacheable since it is used when generating the response. Cache-Control headers are unidirectional and response generated by a request with no-cache can be used to update the existing entry in the cache.

Tratcher · 2016-11-30T18:09:04Z

In other words, the request header Cache-Control: no-cache says do not retrieve the resource from cache, it does not prohibit caching the new response.

colltoaction · 2016-11-30T18:43:56Z

Should clients be telling the server when not to use the cache? Before using this middleware my implementation was based on IMemoryCache, and I never thought about checking the headers because it was an implementation detail of the server. I've looked around but haven't found specific information.

DamianEdwards · 2016-11-30T19:21:24Z

@tinchou when it comes to HTTP-based response caching, yes, the client still maintains ability to send cache control headers, thus affecting upstream caching behavior. As a result, you shouldn't rely on whole response caching to control load on your application. Using IMemoryCache or the <cache> Tag Helper are ways you can cache parts of the your application logic and fragments of HTML responses separately from systems that use HTTP cache control headers.

The old docs for ASP.NET Caching (from the System.Web days) covers the differences quite nicely in this introduction.

All that said, it might still be nice to have the response caching middleware support a mode whereby it ignores all client-based HTTP cache control headers, and instead acts as a completely server-controlled cache.

colltoaction · 2016-11-30T19:51:45Z

@DamianEdwards yes, I think that would be my preferred scenario. Since I'm already encoding caching information in my ResponseCache attribute, it'd be really nice if I could have server-side caching automatically set up too.

But if the client can void my cache as they please, then that defeats the whole performance story.

EDIT:

I'm curious, why would anyone want the server to ignore the cache when told by the client?

DamianEdwards · 2016-11-30T21:20:16Z

Because that's normally how HTTP-based caching works. There's a client, server, and any amount of intermediaries in between, and they all speak HTTP and participate in caching, using the same cache control headers.

We should investigate the caching behavior of the popular edge caching appliances & services, along with other frameworks, to see how they differentiate these concerns.

guardrex · 2016-11-30T23:35:05Z

The Azure CDN doesn't respect an invalidation attempt using the Cache-Control header.

Initial request for a stale image serves from the origin ...

HTTP/1.1 200 OK
Cache-Control: public,max-age=2592000
Content-Type: image/png
Date: Wed, 30 Nov 2016 23:15:08 GMT
Etag: 0x8D39DF4E61FD969
Last-Modified: Sun, 26 Jun 2016 19:05:54 GMT
Server: Windows-Azure-Blob/1.0 Microsoft-HTTPAPI/2.0
x-ms-blob-type: BlockBlob
x-ms-lease-status: unlocked
x-ms-meta-CbModifiedTime: Fri, 24 Oct 2014 23:37:07 GMT
x-ms-request-id: fdf3f5e1-0001-0028-6f5f-4b9658000000
x-ms-version: 2009-09-19
Content-Length: 12787

Second request with Cache-Control: no-cache is not respected by the CDN and it serves from the edge node. Note X-Cache: HIT and Server ...

HTTP/1.1 200 OK
Accept-Ranges: bytes
Cache-Control: public,max-age=2592000
Content-Type: image/png
Date: Wed, 30 Nov 2016 23:15:13 GMT
Etag: 0x8D39DF4E61FD969
Last-Modified: Sun, 26 Jun 2016 19:05:54 GMT
Server: ECAcc (dfw/56E0)
X-Cache: HIT
x-ms-blob-type: BlockBlob
x-ms-lease-status: unlocked
x-ms-meta-CbModifiedTime: Fri, 24 Oct 2014 23:37:07 GMT
x-ms-request-id: fdf3f5e1-0001-0028-6f5f-4b9658000000
x-ms-version: 2009-09-19
Content-Length: 12787

ctolkien · 2016-12-10T00:17:00Z

The Azure CDN doesn't respect an invalidation attempt using the Cache-Control header.

Neither does CloudFlare.

So I think this behaviour is typical of edge caches.

guardrex · 2016-12-10T00:21:27Z

it might still be nice to have the response caching middleware support a mode whereby it ignores all client-based HTTP cache control headers, and instead acts as a completely server-controlled cache.

👍 I like this idea. If offered, I'll definitely use it.

- Always add IresponseCachingFeatu8re before calling the next middleware #81 - Use If-Modified-Since instead of the incorrect If-Unmodified-Since header #83 - Handle proxy-revalidate in the same way as must-revalidate #83 - Handle max-stale with no specified limit #83 - Bypass cache lookup for no-cache but store the response #83 - Bypass response capturing and buffering when no-store is specified #83 - Replace IsRequestCacheable cache policy with three new independent policy checks to reflect these changes

- Always add IresponseCachingFeatu8re before calling the next middleware #81 - Use If-Modified-Since instead of the incorrect If-Unmodified-Since header #83 - Handle proxy-revalidate in the same way as must-revalidate #83 - Handle max-stale with no specified limit #83 - Bypass cache lookup for no-cache but store the response #83 - Bypass response capturing and buffering when no-store is specified #83 - Replace IsRequestCacheable cache policy with three new independent policy checks to reflect these changes - Modify middleware flow to accommodate cache policy updates

JunTaoLuo · 2017-01-10T22:51:28Z

Fixed in #88. Will need to back port to 1.1.1 in a separate issue.

Tratcher added the bug label Nov 29, 2016

Tratcher added the 1 - Ready label Nov 29, 2016

DamianEdwards added this to the 1.1.1 milestone Nov 29, 2016

muratg assigned JunTaoLuo Nov 29, 2016

JunTaoLuo added 2 - Working and removed 1 - Ready labels Nov 30, 2016

JunTaoLuo mentioned this issue Nov 30, 2016

Always add IResponseCachingFeature before calling the next middleware #82

Closed

Bartmax mentioned this issue Dec 8, 2016

Users are able to make site throw an exception when using ResponseCaching middleware. #85

Closed

guardrex mentioned this issue Dec 14, 2016

Response Caching Middleware doc dotnet/AspNetCore.Docs#2315

Merged

JunTaoLuo mentioned this issue Dec 14, 2016

Restructure middleware flow #88

Merged

4 tasks

JunTaoLuo mentioned this issue Dec 22, 2016

Port fix for Update how certain cache directives are handled to 1.0.x #89

Closed

JunTaoLuo added 3 - Done and removed 2 - Working labels Jan 10, 2017

JunTaoLuo modified the milestones: 1.2.0, 1.1.1 Jan 10, 2017

JunTaoLuo closed this as completed Jan 10, 2017

JunTaoLuo mentioned this issue Jan 10, 2017

Port fix for Update how certain cache directives are handled to 1.1.x #91

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache-Control: no-cache breaks the middleware #81

Cache-Control: no-cache breaks the middleware #81

colltoaction commented Nov 28, 2016

Tratcher commented Nov 29, 2016

colltoaction commented Nov 29, 2016

JunTaoLuo commented Nov 29, 2016

JunTaoLuo commented Nov 30, 2016 •

edited

Tratcher commented Nov 30, 2016

colltoaction commented Nov 30, 2016 via email •

edited

DamianEdwards commented Nov 30, 2016

colltoaction commented Nov 30, 2016 •

edited

DamianEdwards commented Nov 30, 2016

guardrex commented Nov 30, 2016

ctolkien commented Dec 10, 2016

guardrex commented Dec 10, 2016

JunTaoLuo commented Jan 10, 2017

Cache-Control: no-cache breaks the middleware #81

Cache-Control: no-cache breaks the middleware #81

Comments

colltoaction commented Nov 28, 2016

Tratcher commented Nov 29, 2016

colltoaction commented Nov 29, 2016

JunTaoLuo commented Nov 29, 2016

JunTaoLuo commented Nov 30, 2016 • edited

Tratcher commented Nov 30, 2016

colltoaction commented Nov 30, 2016 via email • edited

DamianEdwards commented Nov 30, 2016

colltoaction commented Nov 30, 2016 • edited

DamianEdwards commented Nov 30, 2016

guardrex commented Nov 30, 2016

ctolkien commented Dec 10, 2016

guardrex commented Dec 10, 2016

JunTaoLuo commented Jan 10, 2017

JunTaoLuo commented Nov 30, 2016 •

edited

colltoaction commented Nov 30, 2016 via email •

edited

colltoaction commented Nov 30, 2016 •

edited