-
-
Notifications
You must be signed in to change notification settings - Fork 22.2k
Prevent ETag generation when Cache-Control: no-store is set #6994
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
base: master
Are you sure you want to change the base?
Conversation
jonchurch
left a comment
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 ensures compliance with HTTP caching specifications where no-store prohibits storing the response.
This is not accurate as written. This PR is a perf optimization not a spec alignment. ETAGs can always be sent, Cache-Control: no-store just makes them useless to the client.
Do we have benchmark data showing if this creates a meaningful impact? We pay for a (fast) header check on every response to see if we can save the (also fast) hashing of the body.
|
I don't think it makes meaningful impact but I'll look asap. |
|
We should also review what Doug had mentioned, i haven’t done my investigation on this yet. |
Based on mine measurements, the optimization reduces res.send from ~0.20 ms to ~0.16 ms per request. So it does improve performance, mainly by removing ETag computation, but the overall impact is very small that around 0.04 ms per response. |
Doug's comment is probably based on the
I have looked at Firefox source code and it looks like Based on this, I'd say that ETag is not necessary, but RFC 9111 introduced one more change. At the end of
The Section 5.2.2.3. must-understand says:
If I understand this correctly, |
Respect the Cache-Control: no-store directive by skipping ETag generation when this header is present.
Changes:
This ensures compliance with HTTP caching specifications where no-store prohibits storing the response.
Issue: #2472
Review wanted to @bjohansebas