Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Commit

Permalink
Propagate 'cache-control:public' from inputs to outputs.
Browse files Browse the repository at this point in the history
Partially fixes apache/incubator-pagespeed-ngx#1149

General strategy:
 - adjust general mechanism for computing output response header from input response headers
   to incorporate 'public' on input.

A challenge here is to make sure we test all the egress points, e.g.
  - pagespeed resources: cached and reconstructed on demand
  - fallbacks of various sorts
  - ipro resources: cached and reconstructed on demand
  - loaded from LoadFromFile

Propagate 'cache-control:public' from inputs to outputs.

Partially fixes apache/incubator-pagespeed-ngx#1149

General strategy:
 - adjust general mechanism for computing output response header from input response headers
   to incorporate 'public' on input.

A challenge here is to make sure we test all the egress points, e.g.
  - pagespeed resources: cached and reconstructed on demand
  - fallbacks of various sorts
  - ipro resources: cached and reconstructed on demand
  - loaded from LoadFromFile

fixup

fixup for c++03

more c++03isms

rm unused include

rm override (no c++11 on branch 33)

Also add 'public' to non-private cache-control if request has
via:*google*.

Completes the fix to
apache/incubator-pagespeed-ngx#1149

A challenge here is to make sure we test all the egress points, e.g.
  - pagespeed resources: cached and reconstructed on demand
  - fallbacks of various sorts
  - ipro resources: cached and reconstructed on demand
  - loaded from LoadFromFile
also we must make sure we don't cache the 'public' based on the request
headers.
  • Loading branch information
jmarantz authored and crowell committed Jul 26, 2016
1 parent d4bcfe5 commit f56391f
Show file tree
Hide file tree
Showing 20 changed files with 355 additions and 105 deletions.
9 changes: 7 additions & 2 deletions install/debug.conf.template
Expand Up @@ -105,7 +105,7 @@ ModPagespeedDefaultSharedMemoryCacheKB 0
# in browser caches will stay relevant.
<FilesMatch "\.(jpg|jpeg|gif|png|js|css)$">
Header unset Etag
Header set Cache-control "public, max-age=600"
Header set Cache-control "max-age=600"
</FilesMatch>
</IfModule>
</Directory>
Expand Down Expand Up @@ -940,7 +940,7 @@ NameVirtualHost 127.0.0.1:@@APACHE_TERTIARY_PORT@@

# Make the deadline long here for valgrind tests. We could
# conditionalize this.
ModPagespeedInPlaceRewriteDeadlineMs 10000
ModPagespeedInPlaceRewriteDeadlineMs 20000
</Directory>

<Directory "@@APACHE_DOC_ROOT@@/mod_pagespeed_test/ipro/wait/" >
Expand Down Expand Up @@ -1094,6 +1094,11 @@ ModPagespeedUsePerVHostStatistics on
ModPagespeedDownstreamCacheRebeaconingKey random_rebeaconing_key
</Directory>

<Directory "@@APACHE_DOC_ROOT@@/mod_pagespeed_test/public" >
Header set Cache-control "public,max-age=600"
ModPagespeedPreserveUrlRelativity off
</Directory>

<VirtualHost localhost:@@APACHE_SECONDARY_PORT@@>
ServerName downstreamcacheresource.example.com
DocumentRoot "@@APACHE_DOC_ROOT@@"
Expand Down
1 change: 1 addition & 0 deletions install/mod_pagespeed_test/public/rewrite_css.html
@@ -0,0 +1 @@
<link rel=stylesheet href="yellow.css" />
1 change: 1 addition & 0 deletions install/mod_pagespeed_test/public/yellow.css
@@ -0,0 +1 @@
.yellow {background-color: yellow;}
24 changes: 18 additions & 6 deletions net/instaweb/rewriter/rewrite_context.cc
Expand Up @@ -1066,7 +1066,7 @@ class RewriteContext::FetchContext {
response_headers->CopyFrom(*(
output_resource_->response_headers()));
// Use the most conservative Cache-Control considering all inputs.
ApplyInputCacheControl(response_headers);
AdjustCacheControl();
AddMetadataHeaderIfNecessary(response_headers);
StringPiece contents = output_resource_->ExtractUncompressedContents();
async_fetch_->set_content_length(contents.size());
Expand Down Expand Up @@ -1108,7 +1108,7 @@ class RewriteContext::FetchContext {
// Use the most conservative Cache-Control considering all inputs.
// Note that this is needed because FixFetchFallbackHeaders might
// actually relax things a bit if the input was no-cache.
ApplyInputCacheControl(response_headers);
AdjustCacheControl();
StringPiece contents = input_resource->ExtractUncompressedContents();
ok = rewrite_context_->SendFallbackResponse(
original_output_url_, contents, async_fetch_, handler_);
Expand Down Expand Up @@ -1165,7 +1165,18 @@ class RewriteContext::FetchContext {
rewrite_context_->FixFetchFallbackHeaders(*cached_result,
async_fetch_->response_headers());
// Use the most conservative Cache-Control considering all inputs.
ApplyInputCacheControl(async_fetch_->response_headers());
AdjustCacheControl();

// Add 'public' header if rewritten resource had explicit 'public', which
// happens if the source URLs had 'public'. This is needed for
// ipro-optimized resources, where the actual inputs are used to
// compute the cache-control for a hidden .pagespeed. resource in a
// nested RewriteContext, and we need to propogate that to the ipro
// resource response headers.
if (headers->HasValue(HttpAttributes::kCacheControl, "public")) {
async_fetch_->response_headers()->SetCacheControlPublic();
}

if (!detached_) {
// If we're detached then we don't know what the state of the metadata is
// here as the Rewrite() could still be ongoing in the low-priority
Expand Down Expand Up @@ -1199,14 +1210,15 @@ class RewriteContext::FetchContext {
bool skip_fetch_rewrite() { return skip_fetch_rewrite_; }

private:
void ApplyInputCacheControl(ResponseHeaders* headers) {
void AdjustCacheControl() {
ResourceVector inputs;
for (int i = 0; i < rewrite_context_->num_slots(); i++) {
inputs.push_back(rewrite_context_->slot(i)->resource());
}

rewrite_context_->FindServerContext()->ApplyInputCacheControl(inputs,
headers);
rewrite_context_->FindServerContext()->ApplyInputCacheControl(
inputs, async_fetch_->response_headers());
async_fetch_->FixCacheControlForGoogleCache();
}

RewriteContext* rewrite_context_;
Expand Down
15 changes: 14 additions & 1 deletion net/instaweb/rewriter/rewrite_driver.cc
Expand Up @@ -403,7 +403,19 @@ RewriteDriver* RewriteDriver::Clone() {
result = server_context_->NewRewriteDriverFromPool(pool, request_context_);
}
result->is_nested_ = true;
result->SetRequestHeaders(*request_headers_);

// Remove any Via headers for the nested driver. This is intended for
// removing "Via:1.1 google", so that nested drivers don't wind up
// adding cc:public into intermediate cached results.
//
// Note that we *do* want to propagate http/2 detection to nested drivers.
// This is OK because it gets captured in the RequestContext, which is
// shared, and is not reconstructed from request-headers.
RequestHeaders headers;
headers.CopyFrom(*request_headers_);
headers.RemoveAll(HttpAttributes::kVia);
result->SetRequestHeaders(headers);

return result;
}

Expand Down Expand Up @@ -1831,6 +1843,7 @@ class CacheCallback : public OptionsAwareHTTPCacheCallback {
output_resource_->Link(value, handler_);
output_resource_->SetWritten(true);
async_fetch_->set_content_length(content.size());
async_fetch_->FixCacheControlForGoogleCache();
async_fetch_->HeadersComplete();
success = async_fetch_->Write(content, handler_);
}
Expand Down
34 changes: 34 additions & 0 deletions net/instaweb/rewriter/rewrite_driver_test.cc
Expand Up @@ -193,11 +193,19 @@ TEST_F(RewriteDriverTest, NoChanges) {

TEST_F(RewriteDriverTest, CloneMarksNested) {
RequestHeaders request_headers;
request_headers.Add(HttpAttributes::kAccept, "image/webp");
request_headers.Add("a", "b");
request_headers.Add(HttpAttributes::kVia, "1.1 google");
rewrite_driver()->SetRequestHeaders(request_headers);
RewriteDriver* clone1 = rewrite_driver()->Clone();
EXPECT_TRUE(clone1->is_nested());
EXPECT_TRUE(clone1->request_properties()->SupportsWebpRewrittenUrls());
EXPECT_TRUE(rewrite_driver()->request_headers()->HasValue("a", "b"));
EXPECT_TRUE(clone1->request_headers()->HasValue("a", "b"));
EXPECT_TRUE(rewrite_driver()->request_headers()->HasValue(
HttpAttributes::kVia, "1.1 google"));
EXPECT_FALSE(clone1->request_headers()->HasValue(
HttpAttributes::kVia, "1.1 google"));
clone1->Cleanup();

RewriteDriver* parent2 =
Expand Down Expand Up @@ -439,6 +447,32 @@ TEST_F(RewriteDriverTest, TestCacheUse) {
EXPECT_EQ(0, lru_cache()->num_identical_reinserts());
}

// Test to make sure when we fetch a with a Via header, "public"
// is added to the Cache-Control.
TEST_F(RewriteDriverTest, ViaPublicPageSpeedResource) {
RequestHeaders request_headers;
request_headers.Add(HttpAttributes::kVia, "1.1 google");
AddFilter(RewriteOptions::kRewriteCss);

const char kCss[] = "* { display: none; }";
const char kMinCss[] = "*{display:none}";
SetResponseWithDefaultHeaders("a.css", kContentTypeCss, kCss, 100);

GoogleString url = Encode(kTestDomain, RewriteOptions::kCssFilterId,
hasher()->Hash(kMinCss), "a.css", "css");

// Cold load.
ResponseHeaders response;
GoogleString contents;
ASSERT_TRUE(FetchResourceUrl(url, &request_headers, &contents, &response));
EXPECT_TRUE(response.HasValue(HttpAttributes::kCacheControl, "public"));

// Warm load.
response.Clear();
ASSERT_TRUE(FetchResourceUrl(url, &request_headers, &contents, &response));
EXPECT_TRUE(response.HasValue(HttpAttributes::kCacheControl, "public"));
}

// Extension of above with cache invalidation.
TEST_F(RewriteDriverTest, TestCacheUseWithInvalidation) {
AddFilter(RewriteOptions::kRewriteCss);
Expand Down
2 changes: 1 addition & 1 deletion net/instaweb/rewriter/rewrite_test_base.cc
Expand Up @@ -475,7 +475,7 @@ void RewriteTestBase::DefaultResponseHeaders(
ResponseHeaders* response_headers) {
SetDefaultLongCacheHeaders(&content_type, response_headers);
response_headers->SetDateAndCaching(
timer()->NowMs(), ttl_sec * Timer::kSecondMs, ", public");
timer()->NowMs(), ttl_sec * Timer::kSecondMs);
response_headers->ComputeCaching();
}

Expand Down
14 changes: 11 additions & 3 deletions net/instaweb/rewriter/server_context.cc
Expand Up @@ -445,8 +445,9 @@ void ServerContext::ApplyInputCacheControl(const ResourceVector& inputs,
ResponseHeaders::kHasValidator);

bool browser_cacheable = headers->IsBrowserCacheable();
bool no_store = headers->HasValue(HttpAttributes::kCacheControl,
"no-store");
bool no_store = headers->HasValue(HttpAttributes::kCacheControl, "no-store");
bool is_public = true; // Only used if we see a non-empty resource.
bool saw_nonempty_resource = false;
int64 max_age = headers->cache_ttl_ms();
for (int i = 0, n = inputs.size(); i < n; ++i) {
const ResourcePtr& input_resource(inputs[i]);
Expand All @@ -464,11 +465,18 @@ void ServerContext::ApplyInputCacheControl(const ResourceVector& inputs,
browser_cacheable &= input_headers->IsBrowserCacheable();
no_store |= input_headers->HasValue(HttpAttributes::kCacheControl,
"no-store");
is_public &= input_headers->HasValue(HttpAttributes::kCacheControl,
"public");
saw_nonempty_resource = true;
}
}
DCHECK(!(proxy_cacheable && !browser_cacheable)) <<
"You can't have a proxy-cacheable result that is not browser-cacheable";
if (!proxy_cacheable) {
if (proxy_cacheable) {
if (is_public && saw_nonempty_resource) {
headers->SetCacheControlPublic();
}
} else {
const char* directives = NULL;
if (browser_cacheable) {
directives = ",private";
Expand Down

0 comments on commit f56391f

Please sign in to comment.