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

must-revalidate overspecifies a MUST send 504 instead of 5xx #608

Closed
royfielding opened this issue Dec 11, 2020 · 2 comments · Fixed by #619
Closed

must-revalidate overspecifies a MUST send 504 instead of 5xx #608

royfielding opened this issue Dec 11, 2020 · 2 comments · Fixed by #619

Comments

@royfielding
Copy link
Member

This issue is from a related discussion on Apache httpd, which was trying to comply by sending a 504 even though the revalidation error was a 502. That's nuts.

The current text for must-revalidate says:

The must-revalidate directive is necessary to support reliable operation for certain protocol features. In all circumstances a cache MUST obey the must-revalidate directive; in particular, if a cache is disconnected, the cache MUST generate a 504 (Gateway Timeout) response rather than reuse the stale response.

However, if a proxy/gateway received a 5xx response while trying to revalidate, this requirement forces it to toss that information and supply a useless 504 instead. That isn't desirable. The entire purpose of this requirement is to make sure that the cache does not respond with its stale entry. Hence, any 5xx code is sufficient.

@minfrin
Copy link

minfrin commented Dec 16, 2020

To fill in more detail, the change in question is this one:

apache/httpd@a7fc0f0

And covers the wide question of "which 5xx response is returned in low level network failure cases, such as DNS error, connection refused, connection timed out, etc".

The commit message referred to RFC2616 14.9.4 Cache Revalidation and Reload Controls, but this seems to be a wider issue.

@mnot
Copy link
Member

mnot commented Dec 23, 2020

Yes, that MUST is too strong; I suspect it's there to make the mechanism reliable, not override other potential status codes.

Will work on a PR.

@mnot mnot self-assigned this Dec 23, 2020
mnot added a commit that referenced this issue Dec 31, 2020
reschke added a commit that referenced this issue Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants