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

`If-Modified-Since` not working with IPRO #1106

Closed
jeffkaufman opened this Issue Jul 10, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented Jul 10, 2015

Without PageSpeed:

$ headers -H 'PageSpeed: off' http://www.jefftk.com/pictures/2014/lily-baritone-tn.jpg | grep Last-Modified
Last-Modified: Fri, 02 Jan 2015 20:36:38 GMT

$ headers -H 'If-Modified-Since: Fri, 02 Jan 2015 20:36:38 GMT"' -H 'PageSpeed: off' http://www.jefftk.com/pictures/2014/lily-baritone-tn.jpg
HTTP/1.1 304 Not Modified

With PageSpeed:

$ headers http://www.jefftk.com/pictures/2014/lily-baritone-tn.jpg | grep Last-Modified
Last-Modified: Fri, 10 Jul 2015 13:11:17 GMT

$ headers -H 'If-Modified-Since: Fri, 10 Jul 2015 13:11:17 GMT"' http://www.jefftk.com/pictures/2014/lily-baritone-tn.jpg
HTTP/1.1 200 OK

ETags do work, and we set them.

I just tested mod_pagespeed as well, and the same problem:

$ headers modpagespeed.com/mod_pagespeed_example/images/Puzzle.jpg | grep 'Etag\|Last-Modified'
Etag: W/"PSA-aj-YsUBdV-i_6"
Last-Modified: Fri, 10 Jul 2015 13:18:34 GMT

$ headers -H 'If-None-Match: W/"PSA-aj-YsUBdV-i_6"' modpagespeed.com/mod_pagespeed_example/images/Puzzle.jpg
HTTP/1.1 304 Not Modified

$ headers -H 'If-Modified-Since: Fri, 10 Jul 2015 13:18:34 GMT' modpagespeed.com/mod_pagespeed_example/images/Puzzle.jpg
HTTP/1.1 200 OK
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 10, 2015

Since we do set etags, one option would be simply to drop Last-Modified from our output. In general I would expect etag to work better than last modified, because if we optimize a resource, it falls out of cache, and then we optimize it again, the new one should have the same etag but won't have the same last-modified.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 10, 2015

I think the relevant code is:

void InPlaceRewriteContext::FetchTryFallback(const GoogleString& url,
                                          const StringPiece& hash) {
  const char* request_etag = async_fetch()->request_headers()->Lookup1(
      HttpAttributes::kIfNoneMatch);
  if (request_etag != NULL && !hash.empty() &&
      (HTTPCache::FormatEtag(StrCat(id(), "-", hash)) == request_etag)) {
    // Serve out a 304.                                                          

This doesn't check If-Modified-Since.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 10, 2015

The code that inserts Last-Modified:

Apache:
  ApacheFetch::HandleHeadersComplete():
    response_headers()->SetLastModified(now_ms);

Nginx:
  ps_simple_handler():
    response_headers.SetLastModified(now_ms);

I don't see what setting LastModified is getting us here. It's not helping with caching, because we don't accept it back on the input. It's even inaccurate for most of the traffic that flows through ApacheFetch/ps_simple_handler since it's not accurate for IPRO.

I think we should just remove it, or at least remove it in cases where ETag is also present.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 10, 2015

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.29 does say:

HTTP/1.1 servers SHOULD send Last-Modified whenever feasible.

but it also says:

The Last-Modified entity-header field indicates the date and time at
which the origin server believes the variant was last modified.

Saying "last modified: just this moment" seems incorrect enough that sending no header would be better.

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Jul 10, 2015

You're reading an obsolete spec.
Better reference:
https://tools.ietf.org/html/rfc7232#section-2.2

Also the code seems to come from http://cl/63168180 which is supposed to be
a refactor.
Previous to it we only really produced it for staticstics pages and like,
for which now_ms makes sense.

On Fri, Jul 10, 2015 at 2:07 PM, Jeff Kaufman notifications@github.com
wrote:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.29 does say:

HTTP/1.1 servers SHOULD send Last-Modified whenever feasible.

but it also says:

The Last-Modified entity-header field indicates the date and time at
which the origin server believes the variant was last modified.

Saying "last modified: just this moment" seems incorrect enough that
sending no header would be better.


Reply to this email directly or view it on GitHub
#1106 (comment)
.

@morlovich

This comment has been minimized.

Copy link
Contributor

morlovich commented Jul 10, 2015

That code review link isn't very useful here, is it? :(

On Fri, Jul 10, 2015 at 2:15 PM, Maksim Orlovich morlovich@google.com
wrote:

You're reading an obsolete spec.
Better reference:
https://tools.ietf.org/html/rfc7232#section-2.2

Also the code seems to come from http://cl/63168180 which is supposed to
be a refactor.
Previous to it we only really produced it for staticstics pages and like,
for which now_ms makes sense.

On Fri, Jul 10, 2015 at 2:07 PM, Jeff Kaufman notifications@github.com
wrote:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.29 does say:

HTTP/1.1 servers SHOULD send Last-Modified whenever feasible.

but it also says:

The Last-Modified entity-header field indicates the date and time at
which the origin server believes the variant was last modified.

Saying "last modified: just this moment" seems incorrect enough that
sending no header would be better.


Reply to this email directly or view it on GitHub
#1106 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jul 10, 2015

Tracing history, it looks like the change to add a Last-Modified of "now" dates back to 2011-01-06 back when this code only processed the statistics. Since our statistics really are generated on demand, it's accurate for them. I think we should only be including Last-Modified for on-demand generated responses (stats, messages) and leaving it off for IPRO.

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