Skip to content

Commit

Permalink
bug #35305 [HttpKernel] Fix stale-if-error behavior, add tests (mpdude)
Browse files Browse the repository at this point in the history 
This PR was squashed before being merged into the 3.4 branch (closes #35305).

Discussion
----------

[HttpKernel] Fix stale-if-error behavior, add tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #24248
| License       | MIT
| Doc PR        |

This PR adds the first tests for `stale-if-error` logic in `HttpCache`.

It also fixes an observation from #24248: For responses that have been cached as `public` with an `ETag` but without a lifetime, in case of an error the stale response will be served forever (= as long as the error persists), even beyond the configured `stale-if-error` grace period.

Furthermore, it tries to improve compliance with RFC 7234: Stale responses must not be sent (under no condition) if one of
* `no-cache`
* `must-revalidate`
* `proxy-revalidate` or
* `s-maxage` (sic) is present.

This can be found in the corresponding chapters of Section 5.2.2 for these directives, but is also summarized in [Section 4.2.4](https://tools.ietf.org/html/rfc7234#section-4.2.4) as

 > A cache MUST NOT generate a stale response if it is prohibited by an explicit in-protocol directive (e.g., by a "no-store" or "no-cache" cache directive, a "must-revalidate" cache-response-directive, or an applicable "s-maxage" or "proxy-revalidate" cache-response-directive; see Section 5.2.2).

Because disabling of `stale-if-error` for `s-maxage` responses probably has a big impact on the usefulness of that feature in practice, it has to be enabled explicitly with a new config setting `strict_smaxage` (defaulting to `false`).

Commits
-------

ad5f427 [HttpKernel] Fix stale-if-error behavior, add tests
  • Loading branch information
@fabpot
fabpot committed Jan 30, 2020
2 parents b90664b + ad5f427 commit e50db1f
Show file tree
Hide file tree
Showing 3 changed files with 190 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpFoundation/Response.php
View file
Expand Up @@ -649,7 +649,7 @@ public function isImmutable()
}

/**
* Returns true if the response must be revalidated by caches.
* Returns true if the response must be revalidated by shared caches once it has become stale.
*
* This method indicates that the response must not be served stale by a
* cache in any circumstance without first revalidating with the origin.
Expand Down
30 changes: 27 additions & 3 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Expand Up @@ -452,13 +452,37 @@ protected function forward(Request $request, $catch = false, Response $entry = n
// always a "master" request (as the real master request can be in cache)
$response = SubRequestHandler::handle($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $catch);

// we don't implement the stale-if-error on Requests, which is nonetheless part of the RFC
if (null !== $entry && \in_array($response->getStatusCode(), [500, 502, 503, 504])) {
/*
* Support stale-if-error given on Responses or as a config option.
* RFC 7234 summarizes in Section 4.2.4 (but also mentions with the individual
* Cache-Control directives) that
*
* A cache MUST NOT generate a stale response if it is prohibited by an
* explicit in-protocol directive (e.g., by a "no-store" or "no-cache"
* cache directive, a "must-revalidate" cache-response-directive, or an
* applicable "s-maxage" or "proxy-revalidate" cache-response-directive;
* see Section 5.2.2).
*
* https://tools.ietf.org/html/rfc7234#section-4.2.4
*
* We deviate from this in one detail, namely that we *do* serve entries in the
* stale-if-error case even if they have a `s-maxage` Cache-Control directive.
*/
if (null !== $entry
&& \in_array($response->getStatusCode(), [500, 502, 503, 504])
&& !$entry->headers->hasCacheControlDirective('no-cache')
&& !$entry->mustRevalidate()
) {
if (null === $age = $entry->headers->getCacheControlDirective('stale-if-error')) {
$age = $this->options['stale_if_error'];
}

if (abs($entry->getTtl()) < $age) {
/*
* stale-if-error gives the (extra) time that the Response may be used *after* it has become stale.
* So we compare the time the $entry has been sitting in the cache already with the
* time it was fresh plus the allowed grace period.
*/
if ($entry->getAge() <= $entry->getMaxAge() + $age) {
$this->record($request, 'stale-if-error');

return $entry;
Expand Down
162 changes: 162 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
View file
Expand Up @@ -1523,6 +1523,168 @@ public function testUsesOriginalRequestForSurrogate()
// Surrogate request
$cache->handle($request, HttpKernelInterface::SUB_REQUEST);
}

public function testStaleIfErrorMustNotResetLifetime()
{
// Make sure we don't accidentally treat the response as fresh (revalidated) again
// when stale-if-error handling kicks in.

$responses = [
[
'status' => 200,
'body' => 'OK',
// This is cacheable and can be used in stale-if-error cases:
'headers' => ['Cache-Control' => 'public, max-age=10', 'ETag' => 'some-etag'],
],
[
'status' => 500,
'body' => 'FAIL',
'headers' => [],
],
[
'status' => 500,
'body' => 'FAIL',
'headers' => [],
],
];

$this->setNextResponses($responses);
$this->cacheConfig['stale_if_error'] = 10;

$this->request('GET', '/'); // warm cache

sleep(15); // now the entry is stale, but still within the grace period (10s max-age + 10s stale-if-error)

$this->request('GET', '/'); // hit backend error
$this->assertEquals(200, $this->response->getStatusCode()); // stale-if-error saved the day
$this->assertEquals(15, $this->response->getAge());

sleep(10); // now we're outside the grace period

$this->request('GET', '/'); // hit backend error
$this->assertEquals(500, $this->response->getStatusCode()); // fail
}

/**
* @dataProvider getResponseDataThatMayBeServedStaleIfError
*/
public function testResponsesThatMayBeUsedStaleIfError($responseHeaders, $sleepBetweenRequests = null)
{
$responses = [
[
'status' => 200,
'body' => 'OK',
'headers' => $responseHeaders,
],
[
'status' => 500,
'body' => 'FAIL',
'headers' => [],
],
];

$this->setNextResponses($responses);
$this->cacheConfig['stale_if_error'] = 10; // after stale, may be served for 10s

$this->request('GET', '/'); // warm cache

if ($sleepBetweenRequests) {
sleep($sleepBetweenRequests);
}

$this->request('GET', '/'); // hit backend error

$this->assertEquals(200, $this->response->getStatusCode());
$this->assertEquals('OK', $this->response->getContent());
$this->assertTraceContains('stale-if-error');
}

public function getResponseDataThatMayBeServedStaleIfError()
{
// All data sets assume that a 10s stale-if-error grace period has been configured
yield 'public, max-age expired' => [['Cache-Control' => 'public, max-age=60'], 65];
yield 'public, validateable with ETag, no TTL' => [['Cache-Control' => 'public', 'ETag' => 'some-etag'], 5];
yield 'public, validateable with Last-Modified, no TTL' => [['Cache-Control' => 'public', 'Last-Modified' => 'yesterday'], 5];
yield 'public, s-maxage will be served stale-if-error, even if the RFC mandates otherwise' => [['Cache-Control' => 'public, s-maxage=20'], 25];
}

/**
* @dataProvider getResponseDataThatMustNotBeServedStaleIfError
*/
public function testResponsesThatMustNotBeUsedStaleIfError($responseHeaders, $sleepBetweenRequests = null)
{
$responses = [
[
'status' => 200,
'body' => 'OK',
'headers' => $responseHeaders,
],
[
'status' => 500,
'body' => 'FAIL',
'headers' => [],
],
];

$this->setNextResponses($responses);
$this->cacheConfig['stale_if_error'] = 10; // after stale, may be served for 10s
$this->cacheConfig['strict_smaxage'] = true; // full RFC compliance for this feature

$this->request('GET', '/'); // warm cache

if ($sleepBetweenRequests) {
sleep($sleepBetweenRequests);
}

$this->request('GET', '/'); // hit backend error

$this->assertEquals(500, $this->response->getStatusCode());
}

public function getResponseDataThatMustNotBeServedStaleIfError()
{
// All data sets assume that a 10s stale-if-error grace period has been configured
yield 'public, no TTL but beyond grace period' => [['Cache-Control' => 'public'], 15];
yield 'public, validateable with ETag, no TTL but beyond grace period' => [['Cache-Control' => 'public', 'ETag' => 'some-etag'], 15];
yield 'public, validateable with Last-Modified, no TTL but beyond grace period' => [['Cache-Control' => 'public', 'Last-Modified' => 'yesterday'], 15];
yield 'public, stale beyond grace period' => [['Cache-Control' => 'public, max-age=10'], 30];

// Cache-control values that prohibit serving stale responses or responses without positive validation -
// see https://tools.ietf.org/html/rfc7234#section-4.2.4 and
// https://tools.ietf.org/html/rfc7234#section-5.2.2
yield 'no-cache requires positive validation' => [['Cache-Control' => 'public, no-cache', 'ETag' => 'some-etag']];
yield 'no-cache requires positive validation, even if fresh' => [['Cache-Control' => 'public, no-cache, max-age=10']];
yield 'must-revalidate requires positive validation once stale' => [['Cache-Control' => 'public, max-age=10, must-revalidate'], 15];
yield 'proxy-revalidate requires positive validation once stale' => [['Cache-Control' => 'public, max-age=10, proxy-revalidate'], 15];
}

public function testStaleIfErrorWhenStrictSmaxageDisabled()
{
$responses = [
[
'status' => 200,
'body' => 'OK',
'headers' => ['Cache-Control' => 'public, s-maxage=20'],
],
[
'status' => 500,
'body' => 'FAIL',
'headers' => [],
],
];

$this->setNextResponses($responses);
$this->cacheConfig['stale_if_error'] = 10;
$this->cacheConfig['strict_smaxage'] = false;

$this->request('GET', '/'); // warm cache
sleep(25);
$this->request('GET', '/'); // hit backend error

$this->assertEquals(200, $this->response->getStatusCode());
$this->assertEquals('OK', $this->response->getContent());
$this->assertTraceContains('stale-if-error');
}
}

class TestKernel implements HttpKernelInterface
Expand Down

0 comments on commit e50db1f

Please sign in to comment.