-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] [HttpCache] Keep s-maxage=0 from ESI sub-responses #41663
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks correct to me.
even if must-revalidate
is at least very close semantically, s-maxage: 0
is a valid value and must not be ignored.
regarding mixed s-maxage and no s-maxage: if we talk about what to send out with the combined response: i would expect the most restrictive result. take the lowest value for s-maxage from all fragments, and if either the main request or the fragment is not that does not need to be in this PR though. i think the bugfix of this PR is valid and simple and should be merged. if we have confusion about how to merge the cache headers from sub requests, that might be a more complicated thing to solve, and also has more risk of side effects and breaking running systems. if there is need to look into that, i suggest we discuss that in a new issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuring s-maxage=0
definitely looks correct to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. It looks good. I just have one questions to make sure this is the correct patch.
src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me 👍
416c5df
to
8319c69
Compare
#41665 will probably merge-conflict with this PR here. I don't know what your workflows look like, but if anybody (@nicolas-grekas?) merges this PR here, I can rebase & resolve the other one. |
8319c69
to
ee7bc02
Compare
Thank you @mpdude. |
Resolved merge conflict on #41665. |
When the
ResponseCacheStrategy
is merging ESI surrogates and the master response, it treatss-maxage=0
as if nos-maxage
has been set.The result is that for a main and a surrogate response that both are
public, s-maxage=0
, the result will only bepublic
, with no further expiration time.https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.2 allows caches to assign a heuristic expiration time when no explicit expiration time has been given but the response has been marked as explicitly cacheable with
public
. Clearly, such a heuristic was not intended or desired whenpublic, s-maxage=0
was given.This PR ensures that
s-maxage=0
is passed along with the resulting response.Some notes on
s-maxage=0
You might argue that
s-maxage=0
does not make sense on a response.According to https://datatracker.ietf.org/doc/html/rfc7234#section-3.2,
s-maxage=0
is a valid setting to ensure that a cached response "cannot be used to satisfy a subsequent request without revalidating it on the origin server".This setting can be used to keep responses in edge caches/CDNs, but to re-validate on every request. The bottom line result can still be faster (304 + response already at the edge vs. fetch response from origin).
To my understanding, the difference between
s-maxage=0
andmust-revalidate
is that a "disconnected" cache (one that cannot contact the origin server) must not use a stale response whenmust-revalidate
is used, but is not prohibited from doing so fors-maxage=0
(https://datatracker.ietf.org/doc/html/rfc7234#section-4.2.4). In other words,must-revalidate
is not exactly the same as (or the "right" way instead of)s-maxage=0
.In the special case of ESI (composite) responses, revalidation is not possible (no
ETag
, noLast-Modified
). But, as explained above, it is still important to pass on the explicit expiration time, instead of having no value for it.