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

CacheHandler fails to send 304 with mod_deflate #9427

Closed
hypeJunction opened this Issue Feb 26, 2016 · 13 comments

Comments

Projects
None yet
2 participants
@hypeJunction
Contributor

hypeJunction commented Feb 26, 2016

I am seeing that all JS files are being reloaded on each page load with simplecache enabled on Apache 2.4 withmod_deflate, mod_headers and mod_filter enabled.

I believe it's a symptom of https://bz.apache.org/bugzilla/show_bug.cgi?id=45023

Solution 1: strip -gzip suffix from Request If-None-Match before comparing it to hash/etag.
Solution 2: do not send Etag headers if mod_deflate is on.

I think I would prefer 2.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

I might have seen this happen with ServeFileHandler, but I think it was no longer an issue after I have rewritten the class to use Symfony\Component\HttpFoundation\Response.

I guess longer term solution 3 would be to update CacheHandler to use Response objects.

Contributor

hypeJunction commented Feb 26, 2016

I might have seen this happen with ServeFileHandler, but I think it was no longer an issue after I have rewritten the class to use Symfony\Component\HttpFoundation\Response.

I guess longer term solution 3 would be to update CacheHandler to use Response objects.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

Background:

  • Cache handler responds with a cacheable view and adds an Etag which is an md5 of the view contents
  • mod_deflate gzips the response body and suffices Etag with -gzip
  • Browser requests a cacheable view, with If-None-Match that contains gzip Etag
  • Cache handler fails to match the Etag of the view to the gziped Etag and serves the file again
  • If Etag is not present, browser just serves the resource from cache, which I guess is a problem with simplecache disabled. So, solution 2 would only work partially?

I don't know enough about gzipping, so maybe someone can chime in on this.

Contributor

hypeJunction commented Feb 26, 2016

Background:

  • Cache handler responds with a cacheable view and adds an Etag which is an md5 of the view contents
  • mod_deflate gzips the response body and suffices Etag with -gzip
  • Browser requests a cacheable view, with If-None-Match that contains gzip Etag
  • Cache handler fails to match the Etag of the view to the gziped Etag and serves the file again
  • If Etag is not present, browser just serves the resource from cache, which I guess is a problem with simplecache disabled. So, solution 2 would only work partially?

I don't know enough about gzipping, so maybe someone can chime in on this.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 26, 2016

Member

I think it's completely safe to simply strip "-gzip" before comparing If-None-Match.

I see nothing in Symfony's http-foundation that would deal with this issue.

With simplecache on we could switch to Last-Modified with If-Modified-Since checking. Doesn't seem necessary though, and we couldn't validate with simplecache off.

Member

mrclay commented Feb 26, 2016

I think it's completely safe to simply strip "-gzip" before comparing If-None-Match.

I see nothing in Symfony's http-foundation that would deal with this issue.

With simplecache on we could switch to Last-Modified with If-Modified-Since checking. Doesn't seem necessary though, and we couldn't validate with simplecache off.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 26, 2016

Member

So the flow would be:

  1. browser requests /cache/...
  2. send response with ETag "123456"
  3. mod_deflate 2.4 rewrites it "123456"-gzip or "123456-gzip" in 2.5
  4. user manually refreshes
  5. browser sends conditional request with If-None-Match "123456"-gzip
  6. we remove -gzip and respond with 304 and ETag "123456"
  7. ...does Apache rewrite this ETag? Unknown, but let's assume the worst:
  8. browser receives 304 and updates its cache to use ETag "123456" (no big deal)
Member

mrclay commented Feb 26, 2016

So the flow would be:

  1. browser requests /cache/...
  2. send response with ETag "123456"
  3. mod_deflate 2.4 rewrites it "123456"-gzip or "123456-gzip" in 2.5
  4. user manually refreshes
  5. browser sends conditional request with If-None-Match "123456"-gzip
  6. we remove -gzip and respond with 304 and ETag "123456"
  7. ...does Apache rewrite this ETag? Unknown, but let's assume the worst:
  8. browser receives 304 and updates its cache to use ETag "123456" (no big deal)
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 26, 2016

Member

It would be safe to just str_replace out -gzip because that can't appear in either of our ETag strategies.

Member

mrclay commented Feb 26, 2016

It would be safe to just str_replace out -gzip because that can't appear in either of our ETag strategies.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

Do we need to resend Etag with 304?

Contributor

hypeJunction commented Feb 26, 2016

Do we need to resend Etag with 304?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 26, 2016

Member

Spec says we have to. Would probably be fine not to, but...

Member

mrclay commented Feb 26, 2016

Spec says we have to. Would probably be fine not to, but...

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

PR #9429
Tested manually with both simplecache enabled and disabled. I get 304 responses as I should.

Contributor

hypeJunction commented Feb 26, 2016

PR #9429
Tested manually with both simplecache enabled and disabled. I get 304 responses as I should.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

Not sure about other browsers, but Chrome keeps sending -gzip Etag even if consecutive 304 response didn't have it.

Contributor

hypeJunction commented Feb 26, 2016

Not sure about other browsers, but Chrome keeps sending -gzip Etag even if consecutive 304 response didn't have it.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

We are not deflating images so there is no issue in file handler.

Contributor

hypeJunction commented Feb 26, 2016

We are not deflating images so there is no issue in file handler.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 26, 2016

Member

We really should fix this everywhere we 304 and in 1.12 (LTS).

Member

mrclay commented Feb 26, 2016

We really should fix this everywhere we 304 and in 1.12 (LTS).

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 26, 2016

Contributor

Sorry, I won't have time for that. Too much to do over the weekend and I am finally taking a week holiday.

Contributor

hypeJunction commented Feb 26, 2016

Sorry, I won't have time for that. Too much to do over the weekend and I am finally taking a week holiday.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 28, 2016

fix(http): cache handler sends 304 responses more reliably
Apache 2.4+ adds `-gzip` to ETags when it applies deflate to a response.
This strips it from request `If-None-Match` headers so we match it more
reliably and send 304 responses.

Fixes #9427

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 29, 2016

fix(http): cache handler sends 304 responses more reliably
Apache 2.4+ adds `-gzip` to ETags when it applies deflate to a response.
This strips it from request `If-None-Match` headers so we match it more
reliably and send 304 responses.

Fixes #9427
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 9, 2016

Contributor

Fixed by #9433

Contributor

hypeJunction commented Mar 9, 2016

Fixed by #9433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment