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

ShallowEtagHeaderFilter should not set ETags to non-cached resources [SPR-11110] #15736

Closed
spring-projects-issues opened this issue Nov 23, 2013 · 9 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Nov 23, 2013

Mickaël Tricot opened SPR-11110 and commented

ETags are used for checking if a resource cached in the browser has been modified or not. So ETags headers should be set only for resources with:

  • header Cache-Control with max-age > 0
    OR
  • header Expires with a date in the future

Affects: 4.0 RC1, 4.0.1

Issue Links:

  • #17022 ShallowEtagHeaderFilter is not Servlet 2.5 compatible
  • #12919 ShallowEtagHeaderFilter should make use of specified content length

Referenced from: commits 6fba829

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2013

Eric Dahl commented

I think you are mistaken on the first two bullets.

HTTP Entities are cached based on two dimensions: freshness (Cache-Control: max-age and Expires) and validation (Last-Modified and Etags). For example, if a response has these headers:

Cache-Control: public, max-age=300
ETag: "tag1"

Then, a typical user-agent will cache the resource and not bother refetching the resource for 5 minutes (300 seconds). Once that time is up, the resource is considered stale, and it'll then issue a request with If-None-Match: "tag1" to validate the resource.

Mark Nottingham has a useful explanation of HTTP caching: http://www.mnot.net/cache_docs/

As far as the third bullet, I'm not certain.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2013

Mickaël Tricot commented

You are right. I went too fast and made mistakes. I will edit the issue.

I should have written that ETag headers should be set only if the resource is cached: header Cache-Control with max-age > 0 OR header Expires with date in the future.

About HTTP methods, I am wrong as well. The problem is different (http://odino.org/don-t-rape-http-if-none-match-the-412-http-status-code/). I'll create another issue.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2013

Mickaël Tricot commented

Or maybe it could be more tolerant and set the ETag header when:

  • if present, the header Cache-Control does not contain no-cache or no-store
  • if present, the header Expires does contain a date in the future

What do you think?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2013

Eric Dahl commented

ETags are still useful for these cases

  • Cache-Control: max-age=0 or Expires: (date not in future)
    • With ETags, it allows the user-agent to store the resource, that is stale from the start. This means that for the next request, the user-agent can revalidate it's freshness and possibly not have to re-download it.
  • Cache-Control: no-cache
    • Contrary to what one might guess, no-cache actually means: [1]

no-cache — forces caches to submit the request to the origin server for validation before releasing a cached copy, every time. This is useful to assure that authentication is respected (in combination with public), or to maintain rigid freshness, without sacrificing all of the benefits of caching.

[1] http://www.mnot.net/cache_docs/#CACHE-CONTROL

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 24, 2013

Mickaël Tricot commented

Alright, so if I understand correctly now, we are only sure that the user-agent should not use ETags when Cache-Control contains no-store. But in that case the user-agent probably knows that it can ignore it. The only drawbacks then are the little CPU overhead and response time degradation due to the hash generation.

Do you think it would be worth it to skip the ETag generation when we know that the resource should not be stored?

Thanks a lot for the explanation and sorry for my misunderstanding.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 6, 2013

Eric Dahl commented

Yeah, that seems reasonable. I expect that many people use this filter by applying it to all requests, which may catch DELETEs/PUTs/POSTs. It seems that there could be some extra logic to skip processing for cases which don't make sense, including the "Cache-Control: no-store".

Sorry it took me a while to respond. I'm not a spring developer, but I work with apps that could benefit from this.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 31, 2014

Brian Clozel commented

Hi Mickaël Tricot and Eric Dahl !

To sum it all up - ShallowEtagHeaderFilter could be improved to skip the ETag generation in the following cases:

  1. the request is not a GET
  2. the response has a Cache-Control: no-store header

Main advantage: saving CPU cycles.

Checking case #1 could make sense, but I'm wondering if case #2 is so rare that checking it for all outgoing responses could actually be a performance drawback.

What do you think?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 1, 2014

Mickaël Tricot commented

Hi Brian Clozel,

I agree with your summary. I think it will save CPU, fasten the response time and reduce the headers size in the response.

Regarding case #2, I would think that it is very cheap to check the Cache-Control header, compared to the whole filter overhead. The best way to figure it out is probably to measure it (with or without this extra-check). Anyway, I don't have a strong opinion about it.

Thanks a lot for taking care of this improvement.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 1, 2014

Eric Dahl commented

I agree with Mickaël Tricot. Case #2 seems worth it for the cases where one could avoid a md5sum in comparison to an if check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants