Problem/Motivation

We had an issue where an error happened on the page, and then anonymous users got the cached error page, from the internal page cache.

Proposed resolution

Prevent caching of responses generated in DefaultExceptionSubscriber.

They set status code 500, but it looks like we cache those anyway.

Should we exclude 5xx from the page cache in general?

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#17 never-cache-server-errrors-2429671-17-interdiff.txt1.75 KBBerdir
#17 never-cache-server-errrors-2429671-17.patch3.12 KBBerdir
#8 interdiff.txt4.28 KBWim Leers
#8 never-cache-server-errrors-2429671-8.patch4.04 KBWim Leers
#3 never-cache-server-errrors-2429671-3.patch3.27 KBBerdir
#3 never-cache-server-errrors-2429671-3-test-only.patch1.58 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +D8 cacheability
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

wow

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
never-cache-server-errrors-2429671-3-test-only.patch1.58 KB
never-cache-server-errrors-2429671-3.patch3.27 KB

Fix and test.

Now I feel OK with setting it to major, because I think it is...

$response->isServerError()++

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Priority: Normal » Major

The last submitted patch, 3: never-cache-server-errrors-2429671-3-test-only.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Looks great :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs review

Not entirely sure about this change. Amazon caches 500s see http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/HTTPSt...

At the very least this should be configurable or maybe inside the response policy so the behaviour can be swapped out.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
FileSize
never-cache-server-errrors-2429671-8.patch4.04 KB
interdiff.txt4.28 KB

Excellent remark & suggestion. Implemented that.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I'm not sure if that is really necessary, this is a regression from 7.x, making it configurable is not :)

We discussed that this could also be a private service, but you prefer to a have a default response policy class, fine either way.

Now we need to someone to RTBC this again ;)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Actually, private service is totally fine by me too. I think I said that in IRC too? Either way works for me.

The sole reason I went for this: symmetry between these two:

  page_cache_request_policy:
    class: Drupal\Core\PageCache\DefaultRequestPolicy
    …
  page_cache_response_policy:
    class: Drupal\Core\PageCache\ChainResponsePolicy
    …

By introducing DefaultResponsePolicy, it's fully symmetrical.

But no strong opinion.

znerol’s picture

znerol CreditAttribution: znerol commented

In my point of view D7 is wrong in this particular case, there is no reason to not cache error responses. We should probably cache them with a low ttl though.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

IMHO, that's tricky, because errors like this often happen only once/due to race conditions during cache clears or so (this is how I noticed it), and those really shouldn't be cached.. but there's no way to know if it's this or e.g. the database that's down :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Priority: Major » Critical
Issue tags: +Needs Drupal 8 critical triage

Berdir suggested over in #2453351: Maintenance mode message ends up in page cache, served endlessly that this one should probably be critical as well, since it's a similar situation. Sounds like it to me, but marking for triage nevertheless.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I think the patch is still ready to go, if someone wants to cache error pages, they can now relatively easy do that with the max-cache changes and replacing the policy implementation, I think. It would probably also require a non-trivial implementation that implements something like an error page heat, to only cache the response if the site is actually down and returns a reproducable error and not just one of those race conditions in twig/caching, of which we have quite a lot right now.

Berdir queued 8: never-cache-server-errrors-2429671-8.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/core.services.yml
@@ -121,7 +121,7 @@ services:
   page_cache_response_policy:
-    class: Drupal\Core\PageCache\ChainResponsePolicy
+    class: Drupal\Core\PageCache\DefaultResponsePolicy
     tags:
       - { name: service_collector, tag: page_cache_response_policy, call: addPolicy}

... I'd honestly prefer simply adding a Default response policy to the already existing chain, instead of altering the chain.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
never-cache-server-errrors-2429671-17.patch3.12 KB
never-cache-server-errrors-2429671-17-interdiff.txt1.75 KB

Fine with me, registering it directly as a private service.

Which works great btw, we should open a follow-up issue to do the same for node/image response and basic_auth request policies (where I forgot to do it when it was added). Saves us 2-3 roundtrips through the container service creation.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Much better!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Opened #2453711: Use public: false for request and response policy services.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 06d920a and pushed to 8.0.x. Thanks!

  • alexpott committed 06d920a on 
    Issue #2429671 by Berdir, Wim Leers: "The website has encountered an...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs Drupal 8 critical triage