Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4frommpdude:allow-smaxage-zero
Jun 23, 2021

Conversation

@mpdude
Copy link
Contributor

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
Doc PR

When theResponseCacheStrategy 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 arepublic, 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 aheuristic expiration time when no explicit expiration time has been given but the response has been marked as explicitly cacheable withpublic. Clearly, such a heuristic wasnot intended or desired whenpublic, s-maxage=0 was given.

This PR ensures thats-maxage=0 is passed along with the resulting response.

Some notes ons-maxage=0

You might argue thats-maxage=0 does not make sense on a response.

According tohttps://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 betweens-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, butis 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 (noETag, noLast-Modified). But, as explained above, it is still important to pass on the explicit expiration time, instead of having no value for it.

Toflar and a-menshchikov reacted with heart emoji
@mpdude
Copy link
ContributorAuthor

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

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

@dbudbu 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 ifmust-revalidate is at least very close semantically,s-maxage: 0 is a valid value and must not be ignored.

@dbu
Copy link
Contributor

dbu commentedJun 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 requestor the fragment is notpublic 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
ContributorAuthor

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

Copy link
Contributor

@ToflarToflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Configurings-maxage=0 definitely looks correct to me 👍

Copy link
Member

@NyholmNyholm 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

@dbudbu 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
Copy link
ContributorAuthor

mpdude commentedJun 21, 2021
edited
Loading

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

Thank you@mpdude.

@fabpotfabpot merged commit4bbd76d intosymfony:4.4Jun 23, 2021
@mpdudempdude deleted the allow-smaxage-zero branchJune 23, 2021 13:04
@mpdude
Copy link
ContributorAuthor

Resolved merge conflict on#41665.

This was referencedJun 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@NyholmNyholmNyholm left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

+2 more reviewers

@dbudbudbu approved these changes

@ToflarToflarToflar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

8 participants

@mpdude@dbu@fabpot@dunglas@Toflar@Nyholm@derrabus@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp