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

IPRO with LoadFromFile on a resource that is already fully optimized severs cc:max-age=300. #989

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 11 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

What steps will reproduce the problem?
1. use ModPagespeedLoadFromFile for static files
2. request any of these static files with browser and inspect caching headers 
or analyze page with pagespeed insight

What is the expected output? What do you see instead?
expected: ressource delivered with caching headers set to values defined for 
this mime-type.
instead: Cache-Control  max-age=300, Expires in 5 minutes...

What version of the product are you using (please check X-Mod-Pagespeed
header)?
mod-pagespeed-beta 1.9.32.1-4238

On what operating system?
Centos 6.5

Which version of Apache?
2.2.15

Which MPM?
prefork



Original issue reported on code.google.com by azurro....@gmail.com on 19 Sep 2014 at 12:00

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

to further clearify this: 
if ressources are rewritten by mod_pagespeed, caching is extended as expected.

BUT 
if the static ressources are allready optimized, files are delivered without 
changes (also url is not changed to respect rewriteDomain) and with caching set 
to 300s as mentioned above.
[ipro: Recompressing image `http://domain.tld/images/dcb3e3eb9c7.jpg' (9591 -> 
9591 bytes) doesn't save space; dropped.]

this issue occured after updating to 1.9.32.1




Original comment by azurro....@gmail.com on 19 Sep 2014 at 1:01

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Thanks for the clarification.  I was mystified the initial report and now I 
think I have a pretty good idea.

Can you confirm whether this worked better in earlier versions of 
mod_pagespeed?  If so, my theory is that this is caused by 
ModPagespeedInPlaceResourceOptimization, which changed from default-off to 
default-on in 1.9.

Also, can you let me know whether you have the 'extend_cache' rewriter enabled, 
and what you set the cache-control to in your Apache config?


Original comment by jmara...@google.com on 19 Sep 2014 at 1:14

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Please ignore my initial assumptions, they are wrong and misleading...,  

1. In fact the issue only occurs for assets requested with XHR-reqests and 
located beneath ModPagespeedLoadFromFile ....
2. Yes, i can confirm this works better in mod-pagespeed-stable, where such 
requests result in a response with the global expires settings.

3. Please see attached screenshot showing response headers for both versions 
and the differences... 



Original comment by azurro....@gmail.com on 19 Sep 2014 at 5:46

Attachments:

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

As a workaround, can you try this in your pagespeed.conf:
  ModPagespeedInPlaceResourceOptimization off
and let me know if this solves the problem?  If so, I think we'll have a 
candidate for our first patch release on 1.9.

Original comment by jmara...@google.com on 19 Sep 2014 at 6:58

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Yes! 
ModPagespeedInPlaceResourceOptimization off 
solves the problem.

Thanks!

Original comment by azurro....@gmail.com on 19 Sep 2014 at 8:33

Attachments:

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Summary was: wrong cache settings on static ressources / 
ModPagespeedLoadFromFile

Great; that's a good temporary workaround for now.

However, we need try to repro fix this bug.  Probably the correct remedy is 
that mod_pagespeed should not handle unrewritten resources in the IPRO path, 
but instead let Apache handle them normally.

Note that when we load resource via LoadFromFile we don't have any way to 
figure out what cache-control would have been sent by Apache.

Original comment by jmara...@google.com on 20 Sep 2014 at 1:22

  • Changed title: IPRO with LoadFromFile on a resource that is already fully optimized severs cc:max-age=300.
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

I would like to propose adding a "Cache-Control:  s-maxage=somethingsmall" for 
unoptimized IPRO responses, to force proxy caches to revalidate at some point 
and force them to fetch the optimized asset in the case where the original 
Cache-Control header has a long TTL.

Original comment by osch...@we-amp.com on 25 Sep 2014 at 8:36

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

I think there are three cases here, all in the context of LoadFromFile:

1. resource is not yet optimized
2. resource cannot be optimized (because origin resource is already optimal)
3. resource is optimized

What you are describing is case #1, and for that I think it makes sense to send 
out headers that will prevent it from being cached in a proxy, at least for a 
long time.  However I think this bug is really about case #2, and it turns out 
we also have a problem with case #3.


Case #2 can be solved, I think, by just declining to handle unoptimizable cases 
in the IPRO flow.

However, I think case #3 is also a problem.  We are using our implicit cache 
TTL rather than the origin TTL when we grab the file with LoadFromFile.  We 
never learn the origin TTL because we don't let Apache handle that request 
normally.  I think to solve this we need a new config file option to set the 
TTL used for IPRO-optimized resources read directly from the file system.


Back to case #1 (and Otto's point) -- currently we will (with LoadFromFile) 
issue the same public 300 second cache lifetime for that too, which I think is 
yet another code path :)

Original comment by jmara...@google.com on 25 Sep 2014 at 2:09

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

For case number 2, this is controled by ImplicitCacheTtlMs.

Setting something like

ModPagespeedImplicitCacheTtlMs 2592000000

should address the issue. (At least it seems to fix it from what I've seen.)

Original comment by jcrowell@google.com on 26 Sep 2014 at 6:22

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Thanks for confirming, Jeff.  azurro.tom: this seems like a plausible 
short-term workaround for you, right?  The question is, are we comfortable 
using this same Option to control two different things:
 1. the TTL we use as a default value for resources that lack any Cache-Control or Expires header
 2. the TTL to use for all LoadFromFile requests independent of what the TTL would be if LoadFromFile
     were not used.

Is this an acceptable long-term workaround?  Or should we add a new option?  
Note that we *don't* have a strategy to figure out what the cache-control would 
have been had mod_pagespeed not served the resource directly from the 
file-system.

Original comment by jmara...@google.com on 26 Sep 2014 at 6:49

  • Changed state: RequestClarification
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

ModPagespeedLoadFromFileCacheTtlMs option added in r4277 to address this issue.

This option, when set will be the default load-from-file cache ttl. If not set, 
it is overridded by the ImplicitCacheTtlMs.

Original comment by jcrowell@google.com on 14 Oct 2014 at 3:00

  • Changed state: Fixed

crowell added a commit that referenced this issue Apr 7, 2015

Backport r4277 for 1.9.32.2
Add load_from_file_cache_ttl_ms option.

Add option to specify the cache ttl for resources loaded from file.
Addresses  issue #989

crowell added a commit that referenced this issue Jun 15, 2015

Backport r4277 for 1.9.32.2
Add load_from_file_cache_ttl_ms option.

Add option to specify the cache ttl for resources loaded from file.
Addresses  issue #989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment