Skip to content

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

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
Merged
merged 1 commit into from
Dec 10, 2019
Merged

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

merged 1 commit into from
Dec 10, 2019

Conversation

mpdude
Copy link
Contributor

@mpdude 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
Copy link
Member

An ETag alone does not give the response a lifetime

Last-Modified doesn't either, does it?

@mpdude
Copy link
Contributor Author

mpdude commented Nov 18, 2019 •
edited
Loading

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
Copy link
Member

Should the PR target 3.4?

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

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
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
Copy link
Contributor Author

mpdude commented Nov 28, 2019

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

@nicolas-grekas
Copy link
Member

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
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
Copy link
Member

nicolas-grekas commented Nov 28, 2019 •
edited
Loading

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

@fabpot
Copy link
Member

fabpot commented Nov 30, 2019

Tests do not pass apparently.

@mpdude
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
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?

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 December 10, 2019 08:49
@nicolas-grekas nicolas-grekas force-pushed the etag-no-must-revalidate branch from bf88c19 to 1b1002b Compare December 10, 2019 08:49
@nicolas-grekas
Copy link
Member

Thank you @mpdude.

nicolas-grekas added a commit that referenced this pull request Dec 10, 2019
@nicolas-grekas
bug #34438 [HttpFoundation] Use Cache-Control: must-revalidate only…
42712ee 
… 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
This was referenced Dec 19, 2019
Merged
Merged
This was referenced Jan 21, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@mpdude @derrabus @nicolas-grekas @fabpot @carsonbot