Add ability to cache by response content, check if response was actually changed even if it expired, return same headers on 304 and 200 #13274
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there any issue or related PR? |
|
@mkurz not that i've seen |
|
I support the change from a conceptual point of view, it's a noticeable improvement. I looked at the commit briefly, I'd suggest to create the hash of the response content in a streamed way (e.g. via a |
…lly changed even if it expired, return same headers on 304 and 200 This brings Cached implementation closer to http spec by: 1. actually using response content as described by https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/ETag 2. preserving headers as described by https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/304 For performance reasons this still uses two cache entries per resource, to avoid retrieving potentially large result content from potentially remote cache. For compatibility reasons new ability is gated via `play.cache.hashResponse` config entry. It should probably become default in the future as the old ETag value based on expiration date (essentially random value) doesn't seem to be useful or specified anywhere in http. The config value and relevant code should be removed then.
|
@alexdupre makes sense, updated |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary reason for this is to increase performance by decreasing the amount of result content returned from server side and utilizing 304 response headers to drive the client to make less requests.
This also brings Cached implementation closer to http spec by:
For performance reasons this still uses two cache entries per resource, to avoid retrieving potentially large result content from potentially remote cache.
For compatibility reasons new ability is gated via
play.cache.hashResponseconfig entry. It should probably become default in the future as the old ETag value based on expiration date (essentially random value) doesn't seem to be useful or specified anywhere in http. The config value and relevant code should be removed then.cc @julienrf as the original author
cc @alexdupre as the most recent functionality contributor