Add a simple test that a cached resource with no expiry is not revali…

…dated. Correct inverted expiry check in revalidation code.
servo · jdm · Nov 12, 2014 · Nov 13, 2014 · Nov 13, 2014 · Nov 14, 2014
commit 09619f14ff12888c9c6e534baf7ab479f837ee10
diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs
@@ -40,6 +40,7 @@ use url::Url;
 //TODO: Range requests
 //TODO: Revalidation rules for query strings
 //TODO: Vary
+//TODO: Fix race between multiple revalidations of same entry
 
 /// The key used to differentiate requests in the cache.
 #[deriving(Clone, Hash, PartialEq, Eq)]
@@ -229,7 +230,7 @@ impl MemoryCache {
     /// Mark a cached request as doomed. Any waiting consumers will immediately receive
     /// an error message or a final body payload. The cache entry is immediately removed.
     pub fn doom_request(&mut self, key: &CacheKey, err: String) {
-        info!("dooming entry for {}", key.url);
+        info!("dooming entry for {} ({})", key.url, err);
         match self.complete_entries.remove(key) {
             Some(_) => return,
             None => (),
@@ -353,7 +354,8 @@ impl MemoryCache {
         let key = CacheKey::new(load_data.clone());
         match self.complete_entries.get(&key) {
             Some(resource) => {
-                if self.base_time + resource.expires >= time::now().to_timespec() {
+                if self.base_time + resource.expires < time::now().to_timespec() {
+                    info!("entry for {} has expired", key.url());
                     return Revalidate(key, ExpiryDate(time::at(self.base_time + resource.expires)));
                 }
 
@@ -364,11 +366,15 @@ impl MemoryCache {
                 }).unwrap_or(false);
 
                 if must_revalidate {
+                    info!("entry for {} must be revalidated", key.url());
                     return Revalidate(key, ExpiryDate(resource.last_validated));
                 }
 
                 match resource.metadata.headers.as_ref().and_then(|headers| headers.etag.as_ref()) {
-                    Some(etag) => return Revalidate(key, Etag(etag.clone())),
+                    Some(etag) => {
+                        info!("entry for {} has an Etag", key.url());
+                        return Revalidate(key, Etag(etag.clone()));
+                    }
                     None => ()
                 }
 

diff --git a/tests/content/netharness.js b/tests/content/netharness.js
@@ -0,0 +1,20 @@
+function assert_requests_made(url, n) {
+    var x = new XMLHttpRequest();
+    x.open('GET', 'stats?' + url, false);
+    x.send();
+    is(parseInt(x.responseText), n, '# of requests for ' + url + ' should be ' + n);
+}
+
+function reset_stats() {
+    var x = new XMLHttpRequest();
+    x.open('POST', 'reset', false);
+    x.send();
+    is(x.status, 200, 'resetting stats should succeed');    
+}
+
+function fetch(url) {
+    var x = new XMLHttpRequest();
+    x.open('GET', url, false);
+    x.send();
+    is(x.status, 200, 'fetching ' + url + ' should succeed ');    
+}
diff --git a/tests/content/resources/helper.html b/tests/content/resources/helper.html
@@ -0,0 +1,2 @@
+<html>
+</html>
diff --git a/tests/content/test_cached_request.html b/tests/content/test_cached_request.html
@@ -0,0 +1,14 @@
+<html>
+<head>
+<script src="harness.js"></script>
+<script src="netharness.js"></script>
+</head>
+<body>
+<script>
+  reset_stats();
+  fetch('resources/helper.html');
+  fetch('resources/helper.html');
+  assert_requests_made('resources/helper.html', 1);
+</script>
+</body>
+</html>
diff --git a/tests/httpserver.py b/tests/httpserver.py
@@ -11,14 +11,27 @@ class CountingRequestHandler(SimpleHTTPRequestHandler):
     def __init__(self, req, client_addr, server):
         SimpleHTTPRequestHandler.__init__(self, req, client_addr, server)
 
-    def do_GET(self):
+    def do_POST(self):
         global requests
         parts = self.path.split('/')
-        if parts[1] == 'stats':
+
+        if parts[1] == 'reset':
+            requests = defaultdict(int)
             self.send_response(200)
             self.send_header('Content-Type', 'text/plain')
-            if parts[2]:
-                body = str(requests['/' + parts[2]])
+            self.send_header('Content-Length', 0)
+            self.end_headers()
+            self.wfile.write('')
+            return
+
+    def do_GET(self):
+        global requests
+        parts = self.path.split('?')
+        if parts[0] == '/stats':
+            self.send_response(200)
+            self.send_header('Content-Type', 'text/plain')
+            if len(parts) > 1:
+                body = str(requests['/' + parts[1]])
             else:
                 body = ''
                 for key, value in requests.iteritems():
@@ -28,13 +41,6 @@ def do_GET(self):
             self.wfile.write(body)
             return
 
-        if parts[1] == 'reset':
-            requests = defaultdict(int)
-            self.send_response(200)
-            self.send_header('Content-Type', 'text/plain')
-            self.send_header('Content-Length', 0)
-            return
-
         requests[self.path] += 1
         path = self.translate_path(self.path)
         headers = path + '^headers'