8000 Stale cache is served forever if exception and no max age · Issue #24248 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Stale cache is served forever if exception and no max age #24248

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
Closed
edhgoose opened this issue Sep 18, 2017 · 4 comments
Closed

Stale cache is served forever if exception and no max age #24248

edhgoose opened this issue Sep 18, 2017 · 4 comments
Labels
Bug HttpKernel Status: Needs Review Unconfirmed

Comments

@edhgoose
Copy link
edhgoose commented Sep 18, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 2.8.14

We have unexpected behaviour which frequently occurs when developing locally where if an exception is introduced into a Twig page, a (previously valid) stale version of the twig page is served. This is because the Symfony cache implements stale-if-error, which is implemented here:

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

Lines 466 to 481 in 776b964

// always a "master" request (as the real master request can be in cache)
$response = $this->kernel->handle($request, HttpKernelInterface::MASTER_REQUEST, $catch);
// FIXME: we probably need to also catch exceptions if raw === true
// we don't implement the stale-if-error on Requests, which is nonetheless part of the RFC
if (null !== $entry && in_array($response->getStatusCode(), array(500, 502, 503, 504))) {
if (null === $age = $entry->headers->getCacheControlDirective('stale-if-error')) {
$age = $this->options['stale_if_error'];
}
if (abs($entry->getTtl()) < $age) {
$this->record($request, 'stale-if-error');
return $entry;
}
}

My understanding of this, particularly line 476, is that stale content will only be served if the ttl of the previously served content is less than 60 (which is the default, as per the below):

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

Line 94 in 776b964

'stale_if_error' => 60,

The problem appears to be that we're not setting a max age (our intention is that the decision as to whether something is up to date or not is purely ETag based) and so the ttl is always null.

According to the RFC (https://tools.ietf.org/html/rfc5861#page-3):

Its value indicates the upper limit to staleness; when the cached response is more stale than the indicated amount, the cached response SHOULD NOT be used to satisfy the request, absent other information.

As abs(null) < 60, line 476 will always be true, this means we will always serve stale content if an exception occurs.

Is this the correct behaviour?

I've implemented a test below which help to illustrate what I'd expect the behaviour to be as I understand it regarding the abs(null) and normal behaviour if max-age is defined - I couldn't see any obvious tests that exercise this currently and I'm happy to open a PR if they're useful.

public function testMaxAgeHeaderMissing()
    {
        $this->setNextResponses([
                [
                'body' => "Hello World",
                'status' => 200,
                'headers' => [
                    'Cache-Control' => 'public',
                    'ETag' => '"12345"',
                ]
            ],
            [
                'body' => "Failure",
                'status' => 500,
                'headers' => [

                ]
            ]
        ]);

        // build initial request
        $this->request('GET', '/');
        $this->assertHttpKernelIsCalled();
        $this->assertEquals(200, $this->response->getStatusCode());
        $this->assertNotNull($this->response->headers->get('ETag'));
        $this->assertNotNull($this->response->headers->get('X-Content-Digest'));
        $this->assertEquals('Hello World', $this->response->getContent());
        $this->assertTraceContains('miss');
        $this->assertTraceContains('store');

        // build subsequent request; exception should be served because there's no max-age
        $this->request('GET', '/');
        $this->assertHttpKernelIsCalled();
        $this->assertEquals(500, $this->response->getStatusCode());
        $this->assertNull($this->response->headers->get('ETag'));
        $this->assertNull($this->response->headers->get('X-Content-Digest'));
        $this->assertEquals('Failure', $this->response->getContent());
        $this->assertTraceContains('stale');
        $this->assertTraceContains('invalid');
    }

    public function testServedStaleFromCacheUntilMaxAgePlusStaleIfErrorDefault()
    {
        $this->setNextResponses([
            [
                'body' => "Hello World",
                'status' => 200,
                'headers' => [
                    'Cache-Control' => 'public, max-age=120',
                    'ETag' => '"12345"',
                ]
            ],

            // All subsequent requests are going to fail and we're testing if they're served from cache

            [
                'body' => "Failure",
                'status' => 500,
                'headers' => [

                ]
            ],
            [
                'body' => "Failure",
                'status' => 500,
                'headers' => [

                ]
            ],
            [
                'body' => "Failure",
                'status' => 500,
                'headers' => [

                ]
            ]
        ]);

        $start = time();

        // build initial request. This will populate the cache
        $this->request('GET', '/');
        $this->assertHttpKernelIsCalled();
        $this->assertEquals(200, $this->response->getStatusCode());
        $this->assertNotNull($this->response->headers->get('ETag'));
        $this->assertNotNull($this->response->headers->get('X-Content-Digest'));
        $this->assertEquals('Hello World', $this->response->getContent());
        $this->assertTraceContains('miss');
        $this->assertTraceContains('store');

        // First failed request will be served from cache as it's inside the Max Age.
        $this->request('GET', '/');
        $this->assertHttpKernelIsNotCalled(); // will serve from cache
        $this->assertEquals(200, $this->response->getStatusCode());
        $this->assertNotNull($this->response->headers->get('ETag'));
        $this->assertNotNull($this->response->headers->get('X-Content-Digest'));
        $this->assertLessThan(120, $this->response->headers->get("age"));
        $this->assertEquals('Hello World', $this->response->getContent());
        $this->assertTraceContains('fresh');

        // expires the cache
        $values = $this->getMetaStorageValues();
        $this->assertCount(1, $values);
        $tmp = unserialize($values[0]);
        // max age + 50 of the 60 stale seconds
        $time = \DateTime::createFromFormat('U', $start - 130); // longer than max-age, but less than max-age + 60 which is the default
        $tmp[0][1]['date'] = $time->format(DATE_RFC2822);
        $r = new \ReflectionObject($this->store);
        $m = $r->getMethod('save');
        $m->setAccessible(true);
        $m->invoke($this->store, 'md'.hash('sha256', 'http://localhost/'), serialize($tmp));

        // build subsequent request; should be served from cache because stale-if-error adds 60 seconds to max-age
        $this->request('GET', '/');
        $this->assertHttpKernelIsCalled(); // Will attempt to rebuild content
        $this->assertEquals(200, $this->response->getStatusCode());
        $this->assertNotNull($this->response->headers->get('ETag'));
        $this->assertNotNull($this->response->headers->get('X-Content-Digest'));

        // The returned content should be ~130 seconds old
        $this->assertLessThan(120+60, $this->response->headers->get("age"));
        $this->assertGreaterThan(120, $this->response->headers->get("age"));

        $this->assertEquals('Hello World', $this->response->getContent());
        $this->assertTraceContains('stale');
        $this->assertTraceContains('stale-if-error');
        $this->assertTraceContains('invalid');
        $this->assertTraceContains('store');

        // expires the cache a bit more
        $values = $this->getMetaStorageValues();
        $this->assertCount(1, $values);
        $tmp = unserialize($values[0]);
        // max age + 50 of the 60 stale seconds
        $time = \DateTime::createFromFormat('U', $start - 190); // longer than max-age + 60
        $tmp[0][1]['date'] = $time->format(DATE_RFC2822);
        $r = new \ReflectionObject($this->store);
        $m = $r->getMethod('save');
        $m->setAccessible(true);
        $m->invoke($this->store, 'md'.hash('sha256', 'http://localhost/'), serialize($tmp));

        // build subsequent request; it's too long since the cache expired. We can't serve the stale content
        $this->request('GET', '/');
        $this->assertHttpKernelIsCalled();
        $this->assertEquals(500, $this->response->getStatusCode());
        $this->assertNull($this->response->headers->get('ETag'));
        $this->assertNull($this->response->headers->get('X-Content-Digest'));
        $this->assertEquals('Failure', $this->response->getContent());
        $this->assertTraceContains('stale');
        $this->assertTraceContains('invalid');
    }
@hracik
Copy link
hracik commented Nov 13, 2019

I can confirm this bug. When exception is thrown, using CacheKernel, no max-age then cache is used instead of display error page.

@mpdude
Copy link
Contributor
mpdude commented Jan 10, 2020 •
edited
Loading

So you're...

  • using the HttpCache with stale-if-error as a config option
  • served a Response that was public, but had no (s)maxAge
  • that Response was stored in the cache (which is correct), and
  • the Response is considered stale right away (b/c maxAge = 0).

The next request revalidates the cached response. This time, the backend gives an error, status code 500 probably.

We'd need to check if the condition

if (abs($entry->getTtl()) < $age) { ... }

is really the right thing here, but anyway, as long as the first (cached) response has been sitting in the cache for less than the configured stale-if-error timeout, it would be correct to return it. That's what stale-if-error is all about...

I do understand that this can be surprising to annoying during development, but right now I have no clever idea if or how this could or should be fixed in the core.

@mpdude
Copy link
Contributor
mpdude commented Jan 10, 2020

I may be overlooking something, but stale-if-error never shows up in the src/Symfony/Component/HttpKernel/Tests/HttpCache directory...

@mpdude mpdude mentioned this issue Jan 10, 2020
Merged
fabpot added a commit that referenced this issue Jan 10, 2020
@fabpot
bug #35299 Avoid stale-if-error in FrameworkBundle's HttpCache if k…
a0b976f 
…ernel.debug = true (mpdude)

This PR was merged into the 3.4 branch.

Discussion
----------

Avoid `stale-if-error` in FrameworkBundle's HttpCache if kernel.debug = true

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

When working with the `HttpCache` in development, error messages may not become visible if a `public` response has been successfully generated for the same URL before.

This is because the `HttpCache` from the `HttpKernel` component by default sets `stale_if_error` to 60 seconds.

At least when using the `HttpCache` subclass from the `FrameworkBundle`, we know about the `kernel.debug` setting and its intention to support local development. In that case, we could set the *default* `stale-if-error` value to 0.

Commits
-------

3a23ec8 Avoid stale-if-error if kernel.debug = true, because it hides errors
@fabpot fabpot closed this as completed Jan 10, 2020
@mpdude mpdude mentioned this issue Jan 10, 2020
Merged
@mpdude
Copy link
Contributor
mpdude commented Jan 10, 2020

See #35305 for additional fixes and tests.

fabpot added a commit that referenced this issue Jan 30, 2020
@fabpot
bug #35305 [HttpKernel] Fix stale-if-error behavior, add tests (mpdude)
e50db1f 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review Unconfirmed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @edhgoose @mpdude @chalasr @carsonbot @hracik
0