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.

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

[HttpFoundation] Use `Cache-Control: must-revalidate` only if explicit lifetime has been given #34438

Merged
merged 1 commit into from Dec 10, 2019

Conversation

@mpdude
Copy link
Contributor

mpdude commented Nov 18, 2019

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

This is really nit-picking: The conservative, safe default for Cache-Control is private, no-cache which means the response must not be served from cache unless it has been validated.

If Last-Modified or Expires are present, we can relax no-cache to be must-revalidate, which means that once the response has become stale, it must be revalidated.

An ETag alone does not give the response a lifetime, so IMO sticking with no-cache in this case would be more consistent.

@mpdude mpdude changed the title Don't change to Cache-Control: private, must-revalidate if only the ETag is present [HttpFoundation] Use `Cache-Control: must-revalidate` only if explicit lifetime has been given Nov 18, 2019
@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 18, 2019

An ETag alone does not give the response a lifetime

Last-Modified doesn't either, does it?

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Nov 18, 2019

TIL that if the status code is cacheable by default or a response without explicit freshness has been marked as explicitly cacheable (e.g., with a public response directive), then heuristic freshness based on Last-Modified may be applied.

Until now, I always thought this was a browser quirk.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 18, 2019

Should the PR target 3.4?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Nov 18, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 28, 2019

I don't understand the change. When there is an ETag, we should send private, must-revalidate, because that's what an ETag allows: revalidation.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Nov 28, 2019

no-cache is the default, which tells a cache that it must revalidate the resource upon every request. must-revalidate means that revalidation must occur once the response has become stale.

As an ETag alone does not define a lifetime, my suggestion was to switch from no-cache to must-revalidate only if a header is present that defines the lifetime (as Expires or Cache-Control: max-age=...) or that allows for a heuristic expiration to be applied (Last-Modified is provided).

Nit-picking, really.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Nov 28, 2019

I agree the RFC might have chosen more intention-revealing names.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 28, 2019

An ETag without a lifetime is considered as having a zero lifetime by browsers AFAIK.
I don't think no-cache is appropriate: a conditional request is possible today and works, whereas this change would kill this conditional request.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Nov 28, 2019

No, it would not.

If the lifetime is considered to be zero, no-cache and must-revalidate would have the same effect, namely that a revalidation has to happen right from the start/after 0 seconds.

no-store: Prohibits caching, thus no revalidation/conditional requests.

no-cache: Response can be cached, but must be revalidated every time.

must-revalidate: Response may be used without validation as long as it is fresh; must be revalidated afterwards and must not be used when stale.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 28, 2019

Oh, so no-cache <=> must-revalidate,maxage=0 got it. Works then.

@fabpot
fabpot approved these changes Nov 30, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Nov 30, 2019

Tests do not pass apparently.

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Dec 2, 2019

Failing tests come from ResponseCacheStrategy (here).

With the change from this PR, we now have Cache-Control: no-cache in some cases where previously we had Cache-Control: must-revalidate.

If this change applies to an ESI subrequest, ResponseCacheStrategy now decides (here) that the resulting, ESI-tags-merged response can no longer be cached using the expiration model (here).

It thus adds an Expires header and sets it to the response Date. This is where the tests break: Now the lifetime is 0, previously it was null.

I need to think through the ResponseCacheStrategy behaviour because it feels as if there might be a bug. The change here should not make a difference for the decision if something is cacheable or not.

Second, I'll try to remove the Expires header which IMO is not really necessary; not having it might already make tests pass again.

TL;DR: Working on it ;-)

@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Dec 2, 2019

Maybe @nicolas-grekas can confirm that the deps=high build failure is due to the change being in one component, the fix for the test in another one?

…t lifetime has been given
@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 Dec 10, 2019
@nicolas-grekas nicolas-grekas force-pushed the mpdude:etag-no-must-revalidate branch from bf88c19 to 1b1002b Dec 10, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 10, 2019

Thank you @mpdude.

nicolas-grekas added a commit that referenced this pull request Dec 10, 2019
… if explicit lifetime has been given (mpdude)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] Use `Cache-Control: must-revalidate` only if explicit lifetime has been given

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

This is really nit-picking: The conservative, safe default for `Cache-Control` is `private, no-cache` which means the response must not be served from cache unless it has been validated.

If `Last-Modified` or `Expires` are present, we can relax `no-cache` to be `must-revalidate`, which means that _once the response has become stale_, it must be revalidated.

An `ETag` alone does not give the response a lifetime, so IMO sticking with `no-cache` in this case would be more consistent.

Commits
-------

1b1002b [HttpFoundation] Use `Cache-Control: must-revalidate` only if explicit lifetime has been given
@nicolas-grekas nicolas-grekas merged commit 1b1002b into symfony:3.4 Dec 10, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.