Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e50db1f

Browse filesBrowse files
fabpotfabpot
committed
bug #35305 [HttpKernel] Fix stale-if-error behavior, add tests (mpdude)
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
2 parents b90664b + ad5f427 commit e50db1f
Copy full SHA for e50db1f

File tree

3 files changed

+190
-4
lines changed
Filter options

3 files changed

+190
-4
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpFoundation/Response.php
+1-1Lines changed: 1 addition & 1 deletion
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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
+27-3Lines changed: 27 additions & 3 deletions
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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
+162Lines changed: 162 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.