Skip to content

Commit e50db1f

Browse files
fabpotfabpot
committed
bug symfony#35305 [HttpKernel] Fix stale-if-error behavior, add tests (mpdude)
This PR was squashed before being merged into the 3.4 branch (closes symfony#35305). Discussion ---------- [HttpKernel] Fix stale-if-error behavior, add tests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | symfony#24248 | License | MIT | Doc PR | This PR adds the first tests for `stale-if-error` logic in `HttpCache`. It also fixes an observation from symfony#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
2 parents b90664b + ad5f427 commit e50db1f

File tree

3 files changed

+190
-4
lines changed

3 files changed

+190
-4
lines changed

‎src/Symfony/Component/HttpFoundation/Response.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -649,7 +649,7 @@ public function isImmutable()
649649
}
650650

651651
/**
652-
* Returns true if the response must be revalidated by caches.
652+
* Returns true if the response must be revalidated by shared caches once it has become stale.
653653
*
654654
* This method indicates that the response must not be served stale by a
655655
* cache in any circumstance without first revalidating with the origin.

‎src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

+27-3
Original file line numberDiff line numberDiff line change
@@ -452,13 +452,37 @@ protected function forward(Request $request, $catch = false, Response $entry = n
452452
// always a "master" request (as the real master request can be in cache)
453453
$response = SubRequestHandler::handle($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $catch);
454454

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

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

464488
return $entry;

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

+162
Original file line numberDiff line numberDiff line change
@@ -1523,6 +1523,168 @@ public function testUsesOriginalRequestForSurrogate()
15231523
// Surrogate request
15241524
$cache->handle($request, HttpKernelInterface::SUB_REQUEST);
15251525
}
1526+
1527+
public function testStaleIfErrorMustNotResetLifetime()
1528+
{
1529+
// Make sure we don't accidentally treat the response as fresh (revalidated) again
1530+
// when stale-if-error handling kicks in.
1531+
1532+
$responses = [
1533+
[
1534+
'status' => 200,
1535+
'body' => 'OK',
1536+
// This is cacheable and can be used in stale-if-error cases:
1537+
'headers' => ['Cache-Control' => 'public, max-age=10', 'ETag' => 'some-etag'],
1538+
],
1539+
[
1540+
'status' => 500,
1541+
'body' => 'FAIL',
1542+
'headers' => [],
1543+
],
1544+
[
1545+
'status' => 500,
1546+
'body' => 'FAIL',
1547+
'headers' => [],
1548+
],
1549+
];
1550+
1551+
$this->setNextResponses($responses);
1552+
$this->cacheConfig['stale_if_error'] = 10;
1553+
1554+
$this->request('GET', '/'); // warm cache
1555+
1556+
sleep(15); // now the entry is stale, but still within the grace period (10s max-age + 10s stale-if-error)
1557+
1558+
$this->request('GET', '/'); // hit backend error
1559+
$this->assertEquals(200, $this->response->getStatusCode()); // stale-if-error saved the day
1560+
$this->assertEquals(15, $this->response->getAge());
1561+
1562+
sleep(10); // now we're outside the grace period
1563+
1564+
$this->request('GET', '/'); // hit backend error
1565+
$this->assertEquals(500, $this->response->getStatusCode()); // fail
1566+
}
1567+
1568+
/**
1569+
* @dataProvider getResponseDataThatMayBeServedStaleIfError
1570+
*/
1571+
public function testResponsesThatMayBeUsedStaleIfError($responseHeaders, $sleepBetweenRequests = null)
1572+
{
1573+
$responses = [
1574+
[
1575+
'status' => 200,
1576+
'body' => 'OK',
1577+
'headers' => $responseHeaders,
1578+
],
1579+
[
1580+
'status' => 500,
1581+
'body' => 'FAIL',
1582+
'headers' => [],
1583+
],
1584+
];
1585+
1586+
$this->setNextResponses($responses);
1587+
$this->cacheConfig['stale_if_error'] = 10; // after stale, may be served for 10s
1588+
1589+
$this->request('GET', '/'); // warm cache
1590+
1591+
if ($sleepBetweenRequests) {
1592+
sleep($sleepBetweenRequests);
1593+
}
1594+
1595+
$this->request('GET', '/'); // hit backend error
1596+
1597+
$this->assertEquals(200, $this->response->getStatusCode());
1598+
$this->assertEquals('OK', $this->response->getContent());
1599+
$this->assertTraceContains('stale-if-error');
1600+
}
1601+
1602+
public function getResponseDataThatMayBeServedStaleIfError()
1603+
{
1604+
// All data sets assume that a 10s stale-if-error grace period has been configured
1605+
yield 'public, max-age expired' => [['Cache-Control' => 'public, max-age=60'], 65];
1606+
yield 'public, validateable with ETag, no TTL' => [['Cache-Control' => 'public', 'ETag' => 'some-etag'], 5];
1607+
yield 'public, validateable with Last-Modified, no TTL' => [['Cache-Control' => 'public', 'Last-Modified' => 'yesterday'], 5];
1608+
yield 'public, s-maxage will be served stale-if-error, even if the RFC mandates otherwise' => [['Cache-Control' => 'public, s-maxage=20'], 25];
1609+
}
1610+
1611+
/**
1612+
* @dataProvider getResponseDataThatMustNotBeServedStaleIfError
1613+
*/
1614+
public function testResponsesThatMustNotBeUsedStaleIfError($responseHeaders, $sleepBetweenRequests = null)
1615+
{
1616+
$responses = [
1617+
[
1618+
'status' => 200,
1619+
'body' => 'OK',
1620+
'headers' => $responseHeaders,
1621+
],
1622+
[
1623+
'status' => 500,
1624+
'body' => 'FAIL',
1625+
'headers' => [],
1626+
],
1627+
];
1628+
1629+
$this->setNextResponses($responses);
1630+
$this->cacheConfig['stale_if_error'] = 10; // after stale, may be served for 10s
1631+
$this->cacheConfig['strict_smaxage'] = true; // full RFC compliance for this feature
1632+
1633+
$this->request('GET', '/'); // warm cache
1634+
1635+
if ($sleepBetweenRequests) {
1636+
sleep($sleepBetweenRequests);
1637+
}
1638+
1639+
$this->request('GET', '/'); // hit backend error
1640+
1641+
$this->assertEquals(500, $this->response->getStatusCode());
1642+
}
1643+
1644+
public function getResponseDataThatMustNotBeServedStaleIfError()
1645+
{
1646+
// All data sets assume that a 10s stale-if-error grace period has been configured
1647+
yield 'public, no TTL but beyond grace period' => [['Cache-Control' => 'public'], 15];
1648+
yield 'public, validateable with ETag, no TTL but beyond grace period' => [['Cache-Control' => 'public', 'ETag' => 'some-etag'], 15];
1649+
yield 'public, validateable with Last-Modified, no TTL but beyond grace period' => [['Cache-Control' => 'public', 'Last-Modified' => 'yesterday'], 15];
1650+
yield 'public, stale beyond grace period' => [['Cache-Control' => 'public, max-age=10'], 30];
1651+
1652+
// Cache-control values that prohibit serving stale responses or responses without positive validation -
1653+
// see https://tools.ietf.org/html/rfc7234#section-4.2.4 and
1654+
// https://tools.ietf.org/html/rfc7234#section-5.2.2
1655+
yield 'no-cache requires positive validation' => [['Cache-Control' => 'public, no-cache', 'ETag' => 'some-etag']];
1656+
yield 'no-cache requires positive validation, even if fresh' => [['Cache-Control' => 'public, no-cache, max-age=10']];
1657+
yield 'must-revalidate requires positive validation once stale' => [['Cache-Control' => 'public, max-age=10, must-revalidate'], 15];
1658+
yield 'proxy-revalidate requires positive validation once stale' => [['Cache-Control' => 'public, max-age=10, proxy-revalidate'], 15];
1659+
}
1660+
1661+
public function testStaleIfErrorWhenStrictSmaxageDisabled()
1662+
{
1663+
$responses = [
1664+
[
1665+
'status' => 200,
1666+
'body' => 'OK',
1667+
'headers' => ['Cache-Control' => 'public, s-maxage=20'],
1668+
],
1669+
[
1670+
'status' => 500,
1671+
'body' => 'FAIL',
1672+
'headers' => [],
1673+
],
1674+
];
1675+
1676+
$this->setNextResponses($responses);
1677+
$this->cacheConfig['stale_if_error'] = 10;
1678+
$this->cacheConfig['strict_smaxage'] = false;
1679+
1680+
$this->request('GET', '/'); // warm cache
1681+
sleep(25);
1682+
$this->request('GET', '/'); // hit backend error
1683+
1684+
$this->assertEquals(200, $this->response->getStatusCode());
1685+
$this->assertEquals('OK', $this->response->getContent());
1686+
$this->assertTraceContains('stale-if-error');
1687+
}
15261688
}
15271689

15281690
class TestKernel implements HttpKernelInterface

0 commit comments

Comments
 (0)