Skip to content

[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

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Jun 10, 2021

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

When the ResponseCacheStrategy is merging ESI surrogates and the master response, it treats s-maxage=0 as if no s-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 be public, 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 when public, 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 and must-revalidate is that a "disconnected" cache (one that cannot contact the origin server) must not use a stale response when must-revalidate is used, but is not prohibited from doing so for s-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, no Last-Modified). But, as explained above, it is still important to pass on the explicit expiration time, instead of having no value for it.

@mpdude
Copy link
Contributor Author

mpdude commented Jun 10, 2021

@Toflar and @dbu I guess you might have opinions on this.

@mpdude mpdude changed the title [HttpCache] Allow s-maxage=0 and treat it as a valid setting [HttpCache] Pass on s-maxage=0 from ESI sub-responses Jun 10, 2021
@mpdude mpdude changed the title [HttpCache] Pass on s-maxage=0 from ESI sub-responses [HttpCache] Keep s-maxage=0 from ESI sub-responses Jun 10, 2021
@carsonbot carsonbot changed the title [HttpCache] Keep s-maxage=0 from ESI sub-responses [HttpKernel] [HttpCache] Keep s-maxage=0 from ESI sub-responses Jun 10, 2021
Copy link
Contributor

@dbu dbu left a 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.

@dbu
Copy link
Contributor

dbu commented Jun 11, 2021

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 public then the whole response is not public.

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.

@mpdude
Copy link
Contributor Author

mpdude commented Jun 11, 2021

@dbu Indeed it's better to separate the topics; see #41665 for the other issue.

Copy link
Contributor

@Toflar Toflar left a 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 👍

Copy link
Member

@Nyholm Nyholm left a 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.

Copy link
Contributor

@dbu dbu left a 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 👍

@mpdude mpdude force-pushed the allow-smaxage-zero branch from 416c5df to 8319c69 Compare June 21, 2021 06:39
@mpdude
Copy link
Contributor Author

mpdude commented Jun 21, 2021

#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.

@fabpot fabpot force-pushed the allow-smaxage-zero branch from 8319c69 to ee7bc02 Compare June 23, 2021 09:45
@fabpot
Copy link
Member

fabpot commented Jun 23, 2021

Thank you @mpdude.

@fabpot fabpot merged commit 4bbd76d into symfony:4.4 Jun 23, 2021
@mpdude mpdude deleted the allow-smaxage-zero branch June 23, 2021 13:04
@mpdude
Copy link
Contributor Author

mpdude commented Jun 24, 2021

Resolved merge conflict on #41665.

This was referenced Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants