Issue #2699613 by alexpott, catch, Wim Leers, Berdir, dawehner, mpdonadio, borisson_, Fabianx: Set a shorter TTL for 404 responses in page_cache module

core/modules/page_cache/src/StackMiddleware/PageCache.php

@@ -7,6 +7,7 @@ use Drupal\Core\Cache\CacheableResponseInterface;
 use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\PageCache\RequestPolicyInterface;
 use Drupal\Core\PageCache\ResponsePolicyInterface;
+use Drupal\Core\Site\Settings;
 use Symfony\Component\HttpFoundation\BinaryFileResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
@@ -243,15 +244,34 @@ class PageCache implements HttpKernelInterface {
       return $response;
     }
 
-    // The response passes all of the above checks, so cache it.
+    $request_time = $request->server->get('REQUEST_TIME');
+    // The response passes all of the above checks, so cache it. Page cache
+    // entries default to Cache::PERMANENT since they will be expired via cache
+    // tags locally. Because of this, page cache ignores max age.
     // - Get the tags from CacheableResponseInterface per the earlier comments.
     // - Get the time expiration from the Expires header, rather than the
     //   interface, but see https://www.drupal.org/node/2352009 about possibly
     //   changing that.
-    $tags = $response->getCacheableMetadata()->getCacheTags();
-    $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.
+    if ($response->isClientError()) {
+      // Cache for an hour by default. If the 'cache_ttl_4xx' setting is
+      // set to 0 then do not cache the response.
+      $cache_ttl_4xx = Settings::get('cache_ttl_4xx', 3600);
+      if ($cache_ttl_4xx > 0) {
+        $expire = $request_time + $cache_ttl_4xx;
+      }
+    }
+    else {
+      $date = $response->getExpires()->getTimestamp();
+      $expire = ($date > $request_time) ? $date : Cache::PERMANENT;
+    }
+
+    if ($expire === Cache::PERMANENT || $expire > $request_time) {
+      $tags = $response->getCacheableMetadata()->getCacheTags();
+      $this->set($request, $response, $expire, $tags);
+    }
 
     // Mark response as a cache miss.
     $response->headers->set('X-Drupal-Cache', 'MISS');

core/modules/page_cache/src/Tests/PageCacheTest.php

@@ -3,6 +3,7 @@
 namespace Drupal\page_cache\Tests;
 
 use Drupal\Component\Datetime\DateTimePlus;
+use Drupal\Core\Site\Settings;
 use Drupal\Core\Url;
 use Drupal\entity_test\Entity\EntityTest;
 use Drupal\simpletest\WebTestBase;
@@ -341,6 +342,7 @@ class PageCacheTest extends WebTestBase {
       403 => $admin_url,
       404 => $invalid_url,
     ];
+    $cache_ttl_4xx = Settings::get('cache_ttl_4xx', 3600);
     foreach ($tests as $code => $content_url) {
       // Anonymous user, without permissions.
       $this->drupalGet($content_url);
@@ -374,6 +376,35 @@ class PageCacheTest extends WebTestBase {
       $this->drupalGet($content_url);
       $this->assertResponse($code);
       $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
+
+      // Ensure the 'expire' field on the cache entry uses cache_ttl_4xx.
+      $cache_item = \Drupal::service('cache.render')->get($this->getUrl() . ':html');
+      $difference = $cache_item->expire - (int) $cache_item->created;
+      // Given that a second might have passed we cannot be sure that
+      // $difference will exactly equal the default cache_ttl_4xx setting.
+      // Account for any timing difference or rounding errors by ensuring the
+      // value is within 5 seconds.
+      $this->assertTrue(
+        $difference > $cache_ttl_4xx - 5 &&
+        $difference < $cache_ttl_4xx + 5,
+        'The cache entry expiry time uses the cache_ttl_4xx setting.'
+      );
+    }
+
+    // Disable 403 and 404 caching.
+    $settings['settings']['cache_ttl_4xx'] = (object) array(
+      'value' => 0,
+      'required' => TRUE,
+    );
+    $this->writeSettings($settings);
+    \Drupal::service('cache.render')->deleteAll();
+
+    foreach ($tests as $code => $content_url) {
+      // Getting the 404 page twice should still result in a cache miss.
+      $this->drupalGet($content_url);
+      $this->drupalGet($content_url);
+      $this->assertResponse($code);
+      $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS');
     }
   }
 

sites/default/default.settings.php

@@ -420,6 +420,20 @@ $settings['update_free_access'] = FALSE;
  */
 # $settings['omit_vary_cookie'] = TRUE;
 
+
+/**
+ * Cache TTL for client error (4xx) responses.
+ *
+ * Items cached per-URL tend to result in a large number of cache items, and
+ * this can be problematic on 404 pages which by their nature are unbounded. A
+ * fixed TTL can be set for these items, defaulting to one hour, so that cache
+ * backends which do not support LRU can purge older entries. To disable caching
+ * of client error responses set the value to 0. Currently applies only to
+ * page_cache module.
+ */
+# $settings['cache_ttl_4xx'] = 3600;
+
+
 /**
  * Class Loader.
  *