Problem/Motivation

The problem stems from the fact that when using page_cache.module we check the response policy at two points. The first is in FinishResponseSubscriber and the second is later in the request in the PageCache middleware.

The problem with this setup comes from the fact that FinishResponseSubscriber assumes it is basically the last thing to run and changes response headers related to caching. If however one of your response policies is looking at one of those headers when it runs later in the middle ware it can return a different result.

Example:
Response policy to not cache responses with "Vary: Accept" headers. In the first check we deny caching. In the second check the Vary header has been removed and we allow caching, putting the non-cacheable result into the cache.

Proposed resolution

Remaining tasks

Figure out a solution?

User interface changes

API changes

CommentFileSizeAuthor
#7 2478915.patch5.23 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Component: cache system » request processing system
Issue tags: +D8 cacheability
znerol’s picture

If you set a Vary header, then you expect that the response is cached somewhere - otherwise you wouldn't set a Vary header. So, generically speaking such a rule does not make any sense at all.

I gather that the intention of introducing such a rule is to prevent that a response is cached in the internal cache if content negotiation is used on a given path. Because this is a limitation of the internal cache, I think it is acceptable to hard-code that condition there. Note that we already have a hard-coded check for BinaryFileResponse and StreamedResponse for the same reason.

neclimdul’s picture

I was under the impression that we where blocking caching on "Vary: Accept" paths per https://www.drupal.org/node/2364011#comment-9681765. In which case we would want to exclude caching but still send a Vary header. If not we should discuss there.

Regardless, I think a more general solution should be adopted instead of another special case. If ResponsePolicy fails in FinishResponse, the only way PageCache should cache the response is if something explicitly sets the response cacheable between it and PageCache. Consider that FinishResponseSubscriber set's "Cache-control: no-cache" yet PageCache still caches the page. Maybe that's the real bug here.

znerol’s picture

@Crell is wrong about that. We need to add Vary: Accept to responses in order to make caching work with reverse proxies when content negotiation is in use on REST routes. Trying to use content negotiation for Ajax/JavaScript stuff is just not worth it because browsers are broken. Browser callbacks and REST are different things, mixing them will cause problems all over the place.

If ResponsePolicy fails in FinishResponse, the only way PageCache should cache the response is if something explicitly sets the response cacheable between it and PageCache.

If the ResponsePolicy denies caching in FinishResponse, then it also will deny caching in the PageCache middleware unless someone broke it.

Consider that FinishResponseSubscriber set's "Cache-control: no-cache" yet PageCache still caches the page. Maybe that's the real bug here.

The PageCache stores pages if the response policy allows it. The FinishResponseSubscriber adds Cache-Control: public if the response policy allows it and if there is a max-age configured, it adds Cache-Control: no-cache if max-age is not configured. So if the response policy allows caching but max-age is configured to 0, then PageCache stores the response (including the Cache-Control: no-cache header) and that behavior is totally fine.

Edit: second part of first paragraph was an early morning fail.

Wim Leers’s picture

neclimdul’s picture

FileSize
5.23 KB

If the ResponsePolicy denies caching in FinishResponse, then it also will deny caching in the PageCache middleware unless someone broke it.

Someone broke it then? Not really trying to point blame but that assumption is currently not true.

The failure of the attached patch should show this. The response policy will return DENY the first call because there is a vary header and null the second time since there are no vary headers. Despite FinishResponse denying caching, PageCache still caches the request because the policy couldn't DENY the second time because the state of the system is different, the response headers have been changed.

The way I see it we have 2 options.

1) Don't mange cache headers in FinishResponseSubscriber
2) We change something so that FinishResponseSubscriber::setResponseNotCacheable() forces responses not to be cached by the PageCache middleware.

neclimdul’s picture

Status: Active » Needs review

to testbot to show failure.

Status: Needs review » Needs work

The last submitted patch, 7: 2478915.patch, failed testing.

znerol’s picture

+++ b/core/modules/page_cache/tests/modules/page_cache_response_policy_test/src/ResponsePolicy/VaryResponse.php
@@ -0,0 +1,27 @@
+    if (in_array('Accept', $response->getVary())) {
+      return static::DENY;
+    }

Just do not do that for the reasons stated above. That rule does not make any sense at all.

Update:

1) Don't mange cache headers in FinishResponseSubscriber

The PageCache middleware stores whole responses (including cache headers). Therefore we somehow need to set cache headers before the page cache middleware handles the response.

2) We change something so that FinishResponseSubscriber::setResponseNotCacheable() forces responses not to be cached by the PageCache middleware.

Like pointed out above, there are legit use-cases where pages are cached in the internal cache even though caching is denied for external caches (browsers, proxies).

Wim Leers’s picture

We lost track of this, but I think we should look at this again now that we're approaching release.

neclimdul’s picture

Yeah, I never knew how to respond to znerol's comments. The point of the issue was response policies are used to determine what should and shouldn't be cached so they should have access to unmodified headers. But he seemed to disagree with the premise so I don't have a path forward.

Originally this was to support real content negotiation support in core but that's taken a different direction so I don't know what interest is outside me still wanting to implement that in contrib at some point.

Wim Leers’s picture

I don't think you're the only one who wants Accept header negotiation support in D8 (contrib).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs work » Closed (works as designed)

I'm closing this issue. Feel free to reopen, but then please include a failing test that demonstrates the problem.