Author: gagan
Date: Fri Feb 11 16:05:54 2011
New Revision: 1069855
URL: http://svn.apache.org/viewvc?rev=1069855&view=rev
Log:
Patch by nikhilmadan23 | Issue 4001053: Reserve nodes when response has
Cache-Control=private or Set-Cookie in CacheEnforcementVisitor |
http://codereview.appspot.com/4001053/
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java?rev=1069855&r1=1069854&r2=1069855&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
Fri Feb 11 16:05:54 2011
@@ -121,6 +121,12 @@ public final class HttpResponse implemen
// Default TTL for an entry in the cache that does not have any cache
control headers.
static final long DEFAULT_TTL = 5L * 60L * 1000L;
+ // TTL to use for strict no-cache response. A value of -1 indicates that a
strict no-cache
+ // resource is never cached. This is also used to indicate if strict
no-cache responses should
+ // be considered as expired. A non-negative value indicates that such
responses should not be
+ // considered as expired as long as they are haven't expired otherwise.
+ static final long DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL = -1;
+
static final Charset DEFAULT_ENCODING = Charset.forName("UTF-8");
@Inject(optional = true) @Named("shindig.cache.http.negativeCacheTtl")
@@ -132,6 +138,9 @@ public final class HttpResponse implemen
@Inject(optional = true) @Named("shindig.http.fast-encoding-detection")
private static boolean fastEncodingDetection = true;
+ @Inject(optional = true)
@Named("shindig.cache.http.strict-no-cache-resource.max-age")
+ private static long strictNoCacheResourceTtl =
DEFAULT_STRICT_NO_CACHE_RESOURCE_TTL;
+
// Support injection of smarter encoding detection
@Inject(optional = true)
private static EncodingDetector.FallbackEncodingDetector
customEncodingDetector =
@@ -336,7 +345,7 @@ public final class HttpResponse implemen
return date + negativeCacheTtl;
}
- if (isStrictNoCache()) {
+ if (isStrictNoCache() && strictNoCacheResourceTtl < 0) {
return -1;
}
long maxAge = getCacheControlMaxAge();
@@ -419,7 +428,7 @@ public final class HttpResponse implemen
/**
* @return max-age value or -1 if invalid or not set
*/
- private long getCacheControlMaxAge() {
+ public long getCacheControlMaxAge() {
String cacheControl = getHeader("Cache-Control");
if (cacheControl != null) {
String[] directives = StringUtils.split(cacheControl, ',');
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java?rev=1069855&r1=1069854&r2=1069855&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitor.java
Fri Feb 11 16:05:54 2011
@@ -41,15 +41,21 @@ import java.util.logging.Logger;
* Visitor that walks over html tags as specified by {@code resourceTags} and
* reserves html tag nodes whose uri attributes are either not in cache, or are
* in cache, but the response in cache is either stale or an error response. In
- * all the above mentioned cases, we trigger a background fetch for the
- * resource. This visitor should be used by a rewriter in conjuction with other
- * visitors which depend on the uri of the html node being in cache.
+ * all the above mentioned cases except for the error case, we trigger a
+ * background fetch for the resource. This visitor should be used by a rewriter
+ * in conjuction with other visitors which depend on the uri of the html node
+ * being in cache.
+ *
+ * Note that in order to use the CacheEnforcementVisitor effectively, the
+ * shindig property shindig.cache.http.strict-no-cache-resource.max-age should
+ * be set to a positive value, so that strict no-cache resources are stored in
+ * cache with this ttl, and unnecessary fetches are not triggered each time for
+ * such resources.
*
*/
public class CacheEnforcementVisitor extends ResourceMutateVisitor {
- private static final Logger logger = Logger.getLogger(
- CacheEnforcementVisitor.class.getName());
+ private static final Logger logger =
Logger.getLogger(CacheEnforcementVisitor.class.getName());
private final HttpCache cache;
private final RequestPipeline requestPipeline;
private final Executor executor;
@@ -95,15 +101,28 @@ public class CacheEnforcementVisitor ext
* @return The visit status of the node.
*/
protected VisitStatus handleResponseInCache(String uri, HttpResponse
response) {
- // TODO: If the response is strict no cache, then we should reserve the
node.
- // Currently, we will keep triggering fetches for these resources.
- // Also, investigate the Cache-Control=no-cache case. Should we proxy
resources in this case?
- // Also, investigate when Cache-Control is max-age=0. We are currently
triggering a fetch in
- // this case. We should look at the TTL of the original response and if
that is zero, we
- // shouldn't trigger a fetch for the resource.
- if (response.isStale() || response.isError()) {
- // Trigger a fetch if the response is stale or an error, and reserve the
node.
- triggerFetch(uri);
+ if (response.isStale()) {
+ // Reserve the node if the response is stale.
+ if (response.getCacheControlMaxAge() != 0) {
+ // If the cache-control max-age of the original response is zero, it
doesn't make sense to
+ // trigger a pre-fetch for it, since by the time the request for it
comes in, it will
+ // already be stale. Such resources will continuously be prefetched,
but the fetched
+ // response will never be used.
+ // TODO: While we definitely should not be pre-fetching resources with
a max-age of 0, other
+ // cases with a very small max-age(say 1s) should also probably not be
pre-fetched either
+ // since the response might not be usable by the time the actual
request comes in. Also
+ // we should consider the cases with no max-age, but instead an
Expires header which is
+ // close to, or the same as the Date header.
+ triggerFetch(uri);
+ }
+ return VisitStatus.RESERVE_NODE;
+ } else if (response.isStrictNoCache() || response.getHeader("Set-Cookie")
!= null ||
+ response.isError()) {
+ // If the response is strict no-cache, or has a Set-Cookie header or is
an error response,
+ // reserve the node. Do not trigger a fetch, since pre-fetching the
resource doesn't help as
+ // the response will not be cached. Also, for the error case, since we
already set the
+ // ttl for such resources to 30 seconds, we should not keep pre-fetching
these till they
+ // become stale.
return VisitStatus.RESERVE_NODE;
} else {
// Otherwise, we assume the cached response is valid and bypass the node.
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java?rev=1069855&r1=1069854&r2=1069855&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CacheEnforcementVisitorTest.java
Fri Feb 11 16:05:54 2011
@@ -18,20 +18,29 @@
package org.apache.shindig.gadgets.rewrite;
import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.createStrictMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import com.google.common.collect.Maps;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import com.google.inject.name.Names;
import org.apache.shindig.common.uri.Uri;
-import org.apache.shindig.common.cache.LruCacheProvider;
-import org.apache.shindig.common.cache.CacheProvider;
+import org.apache.shindig.gadgets.http.AbstractHttpCache;
+import org.apache.shindig.gadgets.http.HttpCache;
import org.apache.shindig.gadgets.http.HttpRequest;
+import org.apache.shindig.gadgets.http.HttpResponse;
import org.apache.shindig.gadgets.http.HttpResponseBuilder;
import org.apache.shindig.gadgets.http.RequestPipeline;
-import org.apache.shindig.gadgets.http.HttpCache;
-import org.apache.shindig.gadgets.http.DefaultHttpCache;
import org.apache.shindig.gadgets.parse.ParseModule.DOMImplementationProvider;
+
import org.junit.Before;
import org.junit.Test;
import org.w3c.dom.Attr;
@@ -59,8 +68,16 @@ public class CacheEnforcementVisitorTest
executor = Executors.newSingleThreadExecutor();
DOMImplementationProvider domImpl = new DOMImplementationProvider();
doc = domImpl.get().createDocument(null, null, null);
- CacheProvider cacheProvider = new LruCacheProvider(10);
- cache = new DefaultHttpCache(cacheProvider);
+ Module module = new AbstractModule() {
+ public void configure() {
+ binder().bindConstant()
+
.annotatedWith(Names.named("shindig.cache.http.strict-no-cache-resource.max-age"))
+ .to(86400L);
+ requestStaticInjection(HttpResponse.class);
+ }
+ };
+ Injector injector = Guice.createInjector(module);
+ cache = injector.getInstance(TestHttpCache.class);
}
@Test
@@ -72,19 +89,19 @@ public class CacheEnforcementVisitorTest
}
@Test
- public void testStaleImgWithZeroMaxAgeReservedAndFetchTriggered() throws
Exception {
+ public void testStaleImgWithZeroMaxAgeReservedAndFetchNotTriggered() throws
Exception {
cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
new HttpResponseBuilder().setResponseString("test")
.addHeader("Cache-Control", "max-age=0").create());
- checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, true);
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
}
@Test
- public void testImgWithErrorResponseReservedAndFetchTriggered() throws
Exception {
+ public void testImgWithErrorResponseReservedAndFetchNotTriggered() throws
Exception {
cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
new HttpResponseBuilder().setResponseString("test")
.setHttpStatusCode(404).create());
- checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, true);
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
}
@Test
@@ -96,6 +113,8 @@ public class CacheEnforcementVisitorTest
@Test
public void testEmbedImgBypassedAndFetchNotTriggered() throws Exception {
+ // This test checks that non img nodes are always bypassed and fetches are
not triggered for
+ // them, since they aren't in the tags specified in
CacheEnforcementVisitor.
checkVisitBypassedAndFetchTriggered("embed", IMG_URL, true, false);
cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
new
HttpResponseBuilder().setResponseString("test").create());
@@ -107,6 +126,51 @@ public class CacheEnforcementVisitorTest
checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, true);
}
+ @Test
+ public void testImgWithCacheControlPrivateReservedAndFetchNotTriggered()
throws Exception {
+ cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
+ new HttpResponseBuilder().setResponseString("test")
+ .addHeader("Cache-Control", "private").create());
+ // Ensure that the strict no-cache resource is cached.
+ assertTrue(cache.getResponse(new HttpRequest(Uri.parse(IMG_URL))) != null);
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
+ }
+
+ @Test
+ public void testImgWithCacheControlNoCacheReservedAndFetchNotTriggered()
throws Exception {
+
+ cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
+ new HttpResponseBuilder().setResponseString("test")
+ .addHeader("Cache-Control", "no-cache").create());
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
+ }
+
+ @Test
+ public void testImgWithCacheControlNoStoreReservedAndFetchNotTriggered()
throws Exception {
+ cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
+ new HttpResponseBuilder().setResponseString("test")
+ .addHeader("Cache-Control", "no-store").create());
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
+ }
+
+ @Test
+ public void testImgWithPragmaNoCacheReservedAndFetchNotTriggered() throws
Exception {
+ cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
+ new HttpResponseBuilder().setResponseString("test")
+ .addHeader("Pragma", "no-cache").create());
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
+ }
+
+ @Test
+ public void
testImgWithSetCookieButNotStrictNoCacheReservedAndFetchNotTriggered()
+ throws Exception {
+ cache.addResponse(new HttpRequest(Uri.parse(IMG_URL)),
+ new HttpResponseBuilder().setResponseString("test")
+ .addHeader("Cache-Control", "public,max-age=86400")
+ .addHeader("Set-Cookie", "name=val").create());
+ checkVisitBypassedAndFetchTriggered("img", IMG_URL, false, false);
+ }
+
/**
* Checks whether a node with the specified tag and url is bypassed by the
* CacheAwareResourceMutateVisitor, and also whether a fetch is triggered for
@@ -133,10 +197,10 @@ public class CacheEnforcementVisitorTest
node.setAttributeNode(attr);
// Mock the RequestPipeline.
- RequestPipeline requestPipeline = createMock(RequestPipeline.class);
+ RequestPipeline requestPipeline = createStrictMock(RequestPipeline.class);
if (expectFetch) {
expect(requestPipeline.execute(new HttpRequest(Uri.parse(url))))
- .andReturn(new
HttpResponseBuilder().setResponseString("test").create());
+ .andReturn(new
HttpResponseBuilder().setResponseString("test").create()).once();
}
replay(requestPipeline);
@@ -144,6 +208,7 @@ public class CacheEnforcementVisitorTest
expect(config.shouldRewriteURL(IMG_URL)).andReturn(true).anyTimes();
expect(config.shouldRewriteTag("img")).andReturn(true).anyTimes();
replay(config);
+
CacheEnforcementVisitor visitor = new CacheEnforcementVisitor(
config, executor, cache, requestPipeline,
ProxyingVisitor.Tags.SCRIPT, ProxyingVisitor.Tags.STYLESHEET,
@@ -158,4 +223,25 @@ public class CacheEnforcementVisitorTest
assertEquals(expectBypass, status == DomWalker.Visitor.VisitStatus.BYPASS);
}
+
+ private static class TestHttpCache extends AbstractHttpCache {
+ protected final Map<String, HttpResponse> map;
+
+ public TestHttpCache() {
+ map = Maps.newHashMap();
+ }
+
+ public void addResponseImpl(String key, HttpResponse response) {
+ map.put(key, response);
+ }
+
+ public HttpResponse getResponseImpl(String key) {
+ return map.get(key);
+ }
+
+ public HttpResponse removeResponseImpl(String key) {
+ return map.remove(key);
+ }
+ }
+
}
