Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy #26532

Merged
merged 1 commit into from Feb 25, 2019
Merged

[HttpKernel] Correctly merging cache directives in HttpCache/ResponseCacheStrategy #26532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
198 changes: 162 additions & 36 deletions src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
View file
Open in desktop
Expand Up @@ -5,10 +5,6 @@
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* This code is partially based on the Rack-Cache library by Ryan Tomayko,
* which is released under the MIT license.
* (based on commit 02d2b48d75bcb63cf1c0c7149c077ad256542801)
aschempp marked this conversation as resolved.
Show resolved Hide resolved
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/
Expand All @@ -28,30 +24,69 @@
*/
class ResponseCacheStrategy implements ResponseCacheStrategyInterface
{
private $cacheable = true;
/**
* Cache-Control headers that are sent to the final response if they appear in ANY of the responses.
*/
private static $overrideDirectives = ['private', 'no-cache', 'no-store', 'no-transform', 'must-revalidate', 'proxy-revalidate'];

/**
* Cache-Control headers that are sent to the final response if they appear in ALL of the responses.
*/
private static $inheritDirectives = ['public', 'immutable'];

private $embeddedResponses = 0;
private $ttls = [];
private $maxAges = [];
private $isNotCacheableResponseEmbedded = false;
private $age = 0;
private $flagDirectives = [
'no-cache' => null,
'no-store' => null,
'no-transform' => null,
'must-revalidate' => null,
'proxy-revalidate' => null,
'public' => null,
'private' => null,
'immutable' => null,
];
private $ageDirectives = [
'max-age' => null,
's-maxage' => null,
'expires' => null,
];

/**
* {@inheritdoc}
*/
public function add(Response $response)
{
if (!$response->isFresh() || !$response->isCacheable()) {
$this->cacheable = false;
} else {
$maxAge = $response->getMaxAge();
$this->ttls[] = $response->getTtl();
$this->maxAges[] = $maxAge;

if (null === $maxAge) {
$this->isNotCacheableResponseEmbedded = true;
++$this->embeddedResponses;

foreach (self::$overrideDirectives as $directive) {
if ($response->headers->hasCacheControlDirective($directive)) {
$this->flagDirectives[$directive] = true;
}
}

++$this->embeddedResponses;
foreach (self::$inheritDirectives as $directive) {
if (false !== $this->flagDirectives[$directive]) {
$this->flagDirectives[$directive] = $response->headers->hasCacheControlDirective($directive);
}
}

$age = $response->getAge();
$this->age = max($this->age, $age);

if ($this->willMakeFinalResponseUncacheable($response)) {
$this->isNotCacheableResponseEmbedded = true;

return;
}

$this->storeRelativeAgeDirective('max-age', $response->headers->getCacheControlDirective('max-age'), $age);
$this->storeRelativeAgeDirective('s-maxage', $response->headers->getCacheControlDirective('s-maxage') ?: $response->headers->getCacheControlDirective('max-age'), $age);

$expires = $response->getExpires();
$expires = null !== $expires ? $expires->format('U') - $response->getDate()->format('U') : null;
$this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0);
}

/**
Expand All @@ -64,33 +99,124 @@ public function update(Response $response)
return;
}

// Remove validation related headers in order to avoid browsers using
// their own cache, because some of the response content comes from
// at least one embedded response (which likely has a different caching strategy).
if ($response->isValidateable()) {
$response->setEtag(null);
$response->setLastModified(null);
// Remove validation related headers of the master response,
// because some of the response content comes from at least
// one embedded response (which likely has a different caching strategy).
$response->setEtag(null);
$response->setLastModified(null);

$this->add($response);

$response->headers->set('Age', $this->age);

if ($this->isNotCacheableResponseEmbedded) {
$response->setExpires($response->getDate());

if ($this->flagDirectives['no-store']) {
$response->headers->set('Cache-Control', 'no-cache, no-store, must-revalidate');
} else {
$response->headers->set('Cache-Control', 'no-cache, must-revalidate');
}

return;
}

$flags = array_filter($this->flagDirectives);

if (isset($flags['must-revalidate'])) {
$flags['no-cache'] = true;
}

if (!$response->isFresh() || !$response->isCacheable()) {
$this->cacheable = false;
$response->headers->set('Cache-Control', implode(', ', array_keys($flags)));

$maxAge = null;
$sMaxage = null;

if (\is_numeric($this->ageDirectives['max-age'])) {
$maxAge = $this->ageDirectives['max-age'] + $this->age;
aschempp marked this conversation as resolved.
Show resolved Hide resolved
$response->headers->addCacheControlDirective('max-age', $maxAge);
}

if (!$this->cacheable) {
$response->headers->set('Cache-Control', 'no-cache, must-revalidate');
if (\is_numeric($this->ageDirectives['s-maxage'])) {
$sMaxage = $this->ageDirectives['s-maxage'] + $this->age;

return;
if ($maxAge !== $sMaxage) {
aschempp marked this conversation as resolved.
Show resolved Hide resolved
$response->headers->addCacheControlDirective('s-maxage', $sMaxage);
}
}

if (\is_numeric($this->ageDirectives['expires'])) {
$date = clone $response->getDate();
$date->modify('+'.($this->ageDirectives['expires'] + $this->age).' seconds');
$response->setExpires($date);
}
}

$this->ttls[] = $response->getTtl();
$this->maxAges[] = $response->getMaxAge();
/**
* RFC2616, Section 13.4.
*
* @see https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.4
*
* @return bool
aschempp marked this conversation as resolved.
Show resolved Hide resolved
*/
private function willMakeFinalResponseUncacheable(Response $response)
{
// RFC2616: A response received with a status code of 200, 203, 300, 301 or 410
// MAY be stored by a cache […] unless a cache-control directive prohibits caching.
if ($response->headers->hasCacheControlDirective('no-cache')
|| $response->headers->getCacheControlDirective('no-store')
) {
return true;
}

if ($this->isNotCacheableResponseEmbedded) {
$response->headers->removeCacheControlDirective('s-maxage');
} elseif (null !== $maxAge = min($this->maxAges)) {
$response->setSharedMaxAge($maxAge);
$response->headers->set('Age', $maxAge - min($this->ttls));
// Last-Modified and Etag headers cannot be merged, they render the response uncacheable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment does not seem to correspond to the following code, as the following deals with responses not having Last-Modified and Etag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe formulate it as "If an embedded response uses Last-Modified or an Etag, the combined response is not cacheable."

hm, but the code is saying that if we have a success status AND no last modified / etag, it certainly is cacheable.
should we instead say that if there is either etag or last-modified, we return true?

and to simplify reasoning in this method, i suggest flipping this method over to keepsFinalResponseCacheable and switch true / false. and invert the bool where we call the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, the method name was a lengthy discussion and finally was @nicolas-grekas's suggestion.

This step determines that according to RFC, a response is always cacheable if it has one of the given response codes. Not having any cache-control information does not make a response uncacheable, it just does not tell a cache what to do.

Copy link
Contributor

@dbu dbu Dec 17, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok with the method name. i prefer "positive" naming over negations, but the field is also done that way round so it could add confusion.

to make this robust and easier to understand, how about saying that as soon as there is etag or last-modified, we return true. and move the status check code to the very end. instead of return true, return !\in_array($response->getStatusCode(), array(...)); . that would be more explicit i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would not be the same!

  • Your suggestion means that a response with either ETag or Last-Modified willMakeFinalResponseUncacheable. That is not what I (tried to) implement
  • The final response is uncache if
    1. ( it is not a given status code OR it has ETag or Last-Modified )
    2. AND it does not have any other caching info (like max-age).

The implementation just works the opposite way.

  1. If it has a given status code and none of the headers, we already know it will not
    …MakeFinalResponseUncacheable
  2. If either it has a different status code OR it has one of the headers, we must also check for Cache-Control headers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, this is not the same. hm, so if status is 200 and we have no etag/last-modified, we say its ok to cache, even if max-age is set to 0? ah, but then we would still mark the final result with max-age: 0, and same goes for private.

okay, then i agree this is correct, though somewhat counterintuitive. can you mention this in the phpdoc, that cache-control instructions are handled elsewhere?

also, if the fragment has no cache-control header but the master response has max-age: 1 day, would we end up with caching the combined response for a whole day? or should we assume a default max-age when caching something with status 200 and no cache-control instruction? varnish takes 2 minutes in that case, by default...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, so if status is 200 and we have no etag/last-modified, we say its ok to cache, even if max-age is set to 0?

No, we don't say its ok to cache. We just determine that this response does not make the final response uncacheable. The final response can still have no useful caching info and therefore be not cacheable.

if the fragment has no cache-control header but the master response has max-age: 1 day, would we end up with caching the combined response for a whole day?

There is no difference between ESI fragments and the master response. They are all sent to the add method, and the result of ALL responses (regardless of their type) is added in the update method. So if any - regardless if master or fragment - has no mergeable data, nothing will be added to the final response.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, this happens here: https://github.com/symfony/symfony/pull/26532/files#diff-e5a339b48cec0fa0a4d0c5c4c705f568R212 - if one response has no max-age we set that info to false.

so this should indeed be fine. maybe put part of this explanation into the phpdoc?

We just determine that this response does not make the final response uncacheable. The final response can still have no useful caching info and therefore be not cacheable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you port the gist of this discussion into the comment here?

// by default (except if the response also has max-age etc.).
if (\in_array($response->getStatusCode(), [200, 203, 300, 301, 410])
&& null === $response->getLastModified()
&& null === $response->getEtag()
aschempp marked this conversation as resolved.
Show resolved Hide resolved
) {
return false;
}

// RFC2616: A response received with any other status code (e.g. status codes 302 and 307)
// MUST NOT be returned in a reply to a subsequent request unless there are
// cache-control directives or another header(s) that explicitly allow it.
$cacheControl = ['max-age', 's-maxage', 'must-revalidate', 'proxy-revalidate', 'public', 'private'];
foreach ($cacheControl as $key) {
if ($response->headers->hasCacheControlDirective($key)) {
return false;
}
}

if ($response->headers->has('Expires')) {
return false;
}

return true;
}

/**
* Store lowest max-age/s-maxage/expires for the final response.
*
* The response might have been stored in cache a while ago. To keep things comparable,
* we have to subtract the age so that the value is normalized for an age of 0.
*
* If the value is lower than the currently stored value, we update the value, to keep a rolling
* minimal value of each instruction. If the value is NULL, the directive will not be set on the final response.
*
* @param string $directive
* @param int|null $value
* @param int $age
*/
private function storeRelativeAgeDirective($directive, $value, $age)
{
if (null === $value) {
$this->ageDirectives[$directive] = false;
}

if (false !== $this->ageDirectives[$directive]) {
$value = $value - $age;
$this->ageDirectives[$directive] = null !== $this->ageDirectives[$directive] ? min($this->ageDirectives[$directive], $value) : $value;
}
$response->setMaxAge(0);
}
}