Set a shorter TTL for 404 responses in page_cache module

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -312,6 +312,14 @@ protected function get(Request $request, $allow_invalid = FALSE) {
+      // Cache for an hour or the maximum age, which ever is the greater.
+      $expire = REQUEST_TIME + min($max_age, 3600);

I think this should be configurable at least via settings.php.

Also s/greater/lesser/?

Fixed the test - the problem is with how $response->getMaxAge() works. I've also move the logic out of set() since I'm not sure it belongs there. Plus I think if the ttl is set to FALSE we shouldn't make any attempt to cache a 404. I also think we should debate whether or not making Drupal cache 404s is correct - since some form of cache busting always possible if we are.

I was also wondering whether 404s should be a cache redirect to a canonical 404 page - but maybe that would give us more problems.

#10: that reminds me of the 4xx-response cache tag, see #2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created.

I think this is related to #2472335: Support an independent max-age for 4xx responses , while that had a different reasoning, I think the result of that issue is similar to the patch in here.

It discussion with @catch I think we should consider calling this negative cache... see https://en.wikipedia.org/wiki/Negative_cache and http://www.squid-cache.org/Doc/config/negative_ttl/

Also I think rather then futzing stuff in PageCache we should consider setting an expires header in 404 generation.

Also re the redirect idea - I didn't mean to do a page redirect. What I was trying to say is that with 404s we cache the same page over and over again - so should we try to only store one 404 page?

After discussing with @catch here's an approach that renames the setting negative_cache_ttl and uses it for 403 and 404s.

• Because @catch used in #8 :)
• isClientError() makes a tonne of sense.

Actually I think it is a bug that we're not using getMaxAge everywhere here - because that falls back to return $this->getExpires()->format('U') - $this->getDate()->format('U');

Discussed some more with @catch. The expires check in page cache is super confusing. By ignoring max-age PageCache is not a http compatible cache implementation and in fact it can be advantageous that it is not. It allows people to use a varnish implementation that does not support cache tags and have a small max-age whilst storing a permanently cache page in PageCache and clearing it using cache tags. Apparently the expires checking was implemented in #566494: Running cron in a shutdown function breaks the site?!?!

Therefore removing the max-age and not implementing an expires check for 4xx responses.

The expires check in page cache is super confusing.

+1

1. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -248,10 +249,26 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +      $settings_ttl = Settings::get('negative_cache_ttl', 3600);
   

   Adding a new setting… shouldn't this belong on \Drupal\system\Controller\Http4xxController::on404() in combination with #2352009: Bubbling of elements' max-age to the page's headers and the page cache?

   Of course, #2352009 will be hard to achieve due to BC. But I'd urge us to have a @todo here pointing to that issue, stating that this should be removed when we do that.

2. +++ b/sites/default/default.settings.php
   @@ -420,6 +420,19 @@
   + * Cache TTL for client error (4xx) responses.
   

   Why TTL here, why not max-age?

@Wim Leers / #24

1. Because this is an implementation detail of PageCache not telling the outside world how to cache our 400s
2. Because this is not about http max-age but about how PageCache behaves.

Basically page cache using max-age is separate and different issue.

#25: Excellent explanations! I'd like to see point 1 made more explicit then, because the setting you're adding in default.settings.php is not in any way explained to be Page Cache-specific. I'd like both the docs and the name to convey this. I think that'd make it non-confusing/non-ambiguous.

So, for the setting name:

+++ b/sites/default/default.settings.php
@@ -420,6 +420,19 @@
+# $settings['negative_cache_ttl'] = 3600;

s/negative_cache_ttl/page_cache_negative_cache_ttl/

or

s/negative_cache_ttl/page_cache_negative_ttl/

or

s/negative_cache_ttl/page_cache_4xx_ttl/

But how do you know if something written to render cache (or any other cache for that matter) for a 4xx response is actually only relevant for 4xx responses? 4xx responses may just as well use cache/compute things that are relevant in other situations as well.

Sounds interesting. I can't quite imagine how that'd work though.

Fine by me. But can we then at least put that rationale in the documentation (or the change record)?

Discussed with @catch, @effulgentsia, @xjm and @webchick. We decided to keep this critical since it is an avenue for DDOS and could result in data loss.

1. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   -    $date = $response->getExpires()->getTimestamp();
   -    $expire = ($date > time()) ? $date : Cache::PERMANENT;
   -    $this->set($request, $response, $expire, $tags);
   +    $expire = 0;
   +    // 403 and 404 responses can fill non-LRU cache backends and generally are
   +    // likely to have a low cache hit rate. So do not cache them permanently.
   

   Wondering if it would be worth it to still respect the Expires header, and only do the default 403/404 logic if no specific header is set.

   But probably not..

2. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +      // Cache for an hour by default. If the ttl is set to 0 then do not cache
   +      // the response
   +      $settings_ttl = Settings::get('negative_cache_ttl', 3600);
   

   Would it make sense to also respect this for the max-age header that we send out?

   I guess that could be a non-critial follow-up?

3. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +      if ($settings_ttl > 0) {
   +        $expire = REQUEST_TIME + $settings_ttl;
   +      }
   

   Not sure about our guideline for using or not using REQUEST_TIME.

   We started using $_SERVER['REQUEST_TIME'] in some cases, see e.g. \Drupal\Core\Cache\MemoryBackend::getRequestTime() and \Drupal\Core\Path\AliasManager::getRequestTime().

   AFAIK, that is to make unit testing easier, especially if we need to fake the current time. Doesn't apply here I think.

4. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -375,6 +375,22 @@ function testPageCacheAnonymous403404() {
   +      $this->assertResponse($code);
   +      $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
   +    }
   

   Wondering how to test the default expiration time.

   Building the page cache cid for a given page is fairly easy, we could load the cache entry manually and check the expire timestamp.

Ah I see, #2472335: Support an independent max-age for 4xx responses is the issue for the max-age thing.

Here's the requested test.

Thank you, I think this is good to go then?

I think this looks good now, but there's too many nits left to be fixed on commit, so moving back to NW. Sorry.

1. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    // via cache tags locally. They also ignore max age, since this allows the
   +    // local page cache to be treated differently from external proxies or CDNs.
   

   This is a bit misleading: some external proxies (e.g. Varnish) and some CDNs (e.g. Fastly and CloudFlare) also support cache tags. So they can use the same strategy.

2. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +      // Cache for an hour by default. If the ttl is set to 0 then do not cache
   

   Nit: s/ttl/TTL/

3. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -374,6 +375,30 @@ function testPageCacheAnonymous403404() {
   +      // Ensure the expires date on the cache entry uses negative_cache_ttl.
   

   s/expires date/'expire' field on the cache item/

4. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -374,6 +375,30 @@ function testPageCacheAnonymous403404() {
   +      $cache_entry = \Drupal::service('cache.render')->get($this->getUrl() . ':html');
   

   AFAIK we always call this $cache_item?

5. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -374,6 +375,30 @@ function testPageCacheAnonymous403404() {
   +      // Account for any rounding errors by rounding the difference ot the
   

   s/ot/to/

6. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -374,6 +375,30 @@ function testPageCacheAnonymous403404() {
   +      // nearest 10. As long as negative_cache_ttl is a multiple of 10 then this
   

   I don't understand this "multiple of 10" stuff?

7. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -374,6 +375,30 @@ function testPageCacheAnonymous403404() {
   +      // Getting the 404 page twice you still result in a cache miss.
   

   s/you//

8. +++ b/sites/default/default.settings.php
   @@ -420,6 +420,19 @@
   + * backends which do not support LRU can purge older entries. To disable caching
   + * set the value to 0. Currently applies only to page_cache module.
   

   s/To disable caching/To disable caching of client error responses/

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -244,14 +245,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
+      if ($settings_ttl > 0) {
+        $expire = REQUEST_TIME + $settings_ttl;
+      }
+    }

Wouldn't `$request->server->get('REQUEST_TIME')` make more sense here since it is available to avoid the global? Also a few lines down.

re #40 / @Wim Leers (thanks for the review)

1. Change the comment to not talk about reverse proxies / CDNs. Doesn't actually seem that relevant.
2. fixed
3. fixed
4. fixed
5. fixed
6. Made more explicit and change commentary
7. fixed
8. fixed

re #41/ @mpdonadio (thanks for the review) - sure why not - we have the request.

I also improved a variable name.

1. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -243,15 +244,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    // 403 and 404 responses can fill non-LRU cache backends and generally are
   

   Can 403 responses fill a cache? How?

2. +++ b/sites/default/default.settings.php
   @@ -420,6 +420,20 @@
   + * Cache TTL for client error (4xx) responses.
   ...
   +# $settings['negative_cache_ttl'] = 3600;
   

   I read the comments in this issue, such as #15 and #28, but I'm not convinced this is a great name, because:

   1. Per #27, there's nothing in the name that scopes it to HTTP responses or renderings performed as part of an error response. What if we at some point decide to implement a negative cache for entity loads (i.e., cache that there is no entity for a given ID)? Should such caches and all other negative caches use the same TTL as for 4xx responses? A setting with this name would imply yes, but I'm not clear on why that would be desirable. For example, NSCD has a negative TTL setting, for which an hour would be way too long.
   2. If the intention behind the name is to imply a parallel with Squid and other web caches, then consider that Squid warns people that a non-0 negative_ttl violates HTTP spec. But the way we're using it in this patch, it does not violate spec, because of the 4xx-response cache tag that we're able to clear whenever something changes that might affect that response. So using a parallel name might actually create a more harmful implication in people's minds than a helpful association.
   3. Per my comment above, do we even want to use this setting for 403 responses? The title and IS of this issue is about 404. And the problem with respect to 404s for this issue isn't that they're negative responses, but that they're an unbounded set. But don't we also have other unbounded sets? For example, appending arbitrary query strings to URLs with 200 responses. So would a setting name that focuses more on unboundedness than on negativity be better?

Unassigning from myself pending feedback on the above.

I think a more interesting scenario is the one from the meta issue. Migrating an existing site to Drupal 8, and possibly not migrating all features/content.

And then google wakes up and notices the big change and starts to checks all the old URL's. We have tens of thousands of requests due to that.

Re #47.1: Yep, thanks for that example.

Re #47.2: My current thinking is:

• If we think we'll want to scope this setting to only cache items (full responses in this issue plus fragments in #2714227: Use negative_ttl for render cache/url cache context on 404s) specific to 4xx conditions, then I suggest naming it 4xx_cache_ttl.
• If we think we'll want to use the same setting for per-URL cache items generated for URLs that are valid and accessible, but include arbitrary query strings, then I suggest naming it unbounded_url_cache_ttl.

I'm ok 4xx_cache_ttl - does what it says on the tin. However $4xx_cache_ttl does not work as a php variable so I guess we should go with 'cache_ttl_4xx'. I introduced the idea of negative_cache_ttl in #15. I still think 404 is a failure and this is a negative cache but the name is still meaningful.

In discussion with @dawhener we realised that we are using time(), REQUEST_TIME and $request->server->get('REQUEST_TIME'); - that's insane. So We've standardized on $request->server->get('REQUEST_TIME');

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -243,15 +244,33 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
+      $expire = ($date > time()) ? $date : Cache::PERMANENT;
...
+    if ($expire === Cache::PERMANENT || $expire > REQUEST_TIME) {

Things are quite inconsistent / unreadable here. Let's use one variable for a consistent time and not deal with relativity theory.

Thank you alex!

In discussion with @dawhener we realised that we are using time(), REQUEST_TIME and $request->server->get('REQUEST_TIME'); - that's insane. So We've standardized on $request->server->get('REQUEST_TIME');

Thank you. :)

Thanks. I'm pretty happy with this patch now, and am tempted to commit to 8.2.x. However,

+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -243,15 +244,34 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
-    $date = $response->getExpires()->getTimestamp();
-    $expire = ($date > time()) ? $date : Cache::PERMANENT;
+    if ($response->isClientError()) {
...      (nothing that checks getExpires())
+    }
+    else {
+      $date = $response->getExpires()->getTimestamp();
+      $expire = ($date > $request_time) ? $date : Cache::PERMANENT;
+    }

I'm nervous about committing this to 8.1. #26 is a reasonable comment, but are we sure we want to apply that reasoning to the production branch? Especially if there's a tag released between this getting committed and #2352009: Bubbling of elements' max-age to the page's headers and the page cache?

Ticking credit boxes.

Pushed to 8.2.x.

Setting to NR for 8.1.x. Per #56, I think for that branch we might want to add the Expires header checking for the 4xx responses as well, for BC for the 0-2 production sites that might be relying on it. If we agree to do that, might make sense to add that to 8.2 as well to keep them in sync, unless #2352009: Bubbling of elements' max-age to the page's headers and the page cache lands before then.

I agree with @catch - especially considering max-age is set later on (if configured) and preferred by browsers and reverse proxies.

Ticked credit box for @effulgentsia - since his reviews have definitely influenced the direction of this patch.

I don't think we should add any more support for the Expires header than we already have

Ok, but currently in 8.1, an Expires header can control the expiration of all page cache entries: whether 2xx or 4xx. Cherry-picking #59 to 8.1 would mean that a production site would see the Expires header no longer controlling the expiration of 4xx page cache entries. So this isn't about adding "any more" support. It's about whether we're ok with removing existing functionality in a patch release.

the behaviour is completely undocumented

Yes, but developers can read code. If as a developer, you've had a reason to want an expiration for a given page cache entry, you surely would have been able to step through the code and end up finding the PageCache::fetch() method, and seen not only the code for how $expire is determined, but even the code comment of Get the time expiration from the Expires header.

I agree with @catch - especially considering max-age is set later on (if configured) and preferred by browsers and reverse proxies.

Yes, a developer finding the above would surely be puzzled/annoyed that you need max-age for browsers and reverse proxies and Expires for Drupal's page_cache.module. And like with other things one gets puzzled/annoyed by, would eventually accept that that's how Drupal core works, and provide both headers in order to control both things. How does changing page_cache.module to no longer obey a shorter Expires header than $cache_ttl_4xx for 4xx responses in a patch release help those developers? Especially, when fixing page_cache.module to use the max-age header is not yet committed to 8.1, and per #57 probably won't be.

So let's do the simple thing and mark this fixed. I'm slightly concerned about 8.1.x sites being vulnerable to this but if it becomes an issue we can revisit the decision. I think one thing that speaks for ignoring the the expires header is what is the worst that can happen? Nothing it going to be broken.

I think one thing that speaks for ignoring the the expires header is what is the worst that can happen? Nothing it going to be broken.

If an 8.1 site currently has some particular URL that returns a 4xx response with a short Expires header (e.g., <1 hour) for anonymous users, and that response then changes after that expiration time (e.g., maybe becomes not a 4xx due to some time-sensitive condition), with no corresponding clearing of the general 4xx-response cache tag or any other cache tag occurring (because if the condition is based on time, then you don't need a cache tag, right), then currently, such a site works as designed, whereas cherry picking #59 to 8.1.x would make such a URL return its old 4xx response for the global $cache_ttl_4xx duration, even if that's longer than the URL-specific time-based condition. This might be an edge case that 0 production sites currently have, but we don't know if that's the case. If such a site has this edge case, then for that site, this would appear as something broken.

Happy to leave this fixed for 8.2 though, and only revisit the BC requirement for 8.1 if we decide we need this in 8.1.

Retroactively unassigning myself (I should have done that in #60).

Also tagging for a release notes mention.