stale-while-revalidate experimental implementation.

net/http/http_cache_unittest.cc

Index: net/http/http_cache_unittest.cc
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 4987c37b025a6aa89f8279586e4b811ba6ad9c30..90679238a39892b714c0cff0b5833d2a9905caa3 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/cache_type.h"
@@ -38,6 +39,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::Time;
+using base::RunLoop;

[rvargas (doing something else), 2014/09/17 04:32:19]

nit: we should probably avoid this and be explicit about the namespace on the
call site.

[Adam Rice, 2014/09/30 13:44:50]

Done. I originally cleaned up every call to RunUntilIdle() in the file, but I
removed those changes to make the CL easy to revert cleanly.

+using base::MessageLoop;

[rvargas (doing something else), 2014/09/17 04:32:19]

not used

[Adam Rice, 2014/09/30 13:44:50]

Removed.

 
 namespace {
 
@@ -6725,6 +6728,43 @@ TEST(HttpCache, ReceivedBytesRange) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+class HttpCacheStaleWhileRevalidateTest : public ::testing::Test {

[rvargas (doing something else), 2014/09/17 04:32:19]

Add a comment about how to use this class (what are the expected windows etc)

[Adam Rice, 2014/09/30 13:44:50]

Done.

+ protected:
+  HttpCacheStaleWhileRevalidateTest()
+      : transaction_(kSimpleGET_Transaction),
+        age_(3601),
+        stale_while_revalidate_(7200),
+        validator_("Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT") {
+    cache_.http_cache()->set_use_stale_while_revalidate_for_testing(true);
+  }
+
+  // RunTransactionTest() with the arguments from this fixure.
+  void RunFixtureTransactionTest() {
+    std::string response_headers = base::StringPrintf(
+        "%s\n"
+        "Age: %d\n"
+        "Cache-Control: max-age=3600,stale-while-revalidate=%d\n",
+        validator_.c_str(),
+        age_,
+        stale_while_revalidate_);
+    transaction_.response_headers = response_headers.c_str();
+    RunTransactionTest(cache_.http_cache(), transaction_);
+    transaction_.response_headers = "";
+  }
+
+  int transaction_count() {
+    return cache_.network_layer()->transaction_count();
+  }
+
+  int open_count() { return cache_.disk_cache()->open_count(); }
+
+  MockHttpCache cache_;
+  ScopedMockTransaction transaction_;
+  int age_;
+  int stale_while_revalidate_;
+  std::string validator_;
+};
+
 static void CheckResourceFreshnessHeader(const net::HttpRequestInfo* request,
                                          std::string* response_status,
                                          std::string* response_headers,
@@ -6736,27 +6776,20 @@ static void CheckResourceFreshnessHeader(const net::HttpRequestInfo* request,
 
 // Verify that the Resource-Freshness header is sent on a revalidation if the
 // stale-while-revalidate directive was on the response.
-TEST(HttpCache, ResourceFreshnessHeaderSent) {
-  MockHttpCache cache;
-
-  ScopedMockTransaction stale_while_revalidate_transaction(
-      kSimpleGET_Transaction);
-  stale_while_revalidate_transaction.response_headers =
-      "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
-      "Age: 10801\n"
-      "Cache-Control: max-age=3600,stale-while-revalidate=7200\n";
+TEST_F(HttpCacheStaleWhileRevalidateTest, ResourceFreshnessHeaderSent) {
+  age_ = 10801;  // Outside the stale-while-revalidate window.
 
   // Write to the cache.
-  RunTransactionTest(cache.http_cache(), stale_while_revalidate_transaction);
+  RunFixtureTransactionTest();
 
-  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, transaction_count());
 
   // Send the request again and check that Resource-Freshness header is added.
-  stale_while_revalidate_transaction.handler = CheckResourceFreshnessHeader;
+  transaction_.handler = CheckResourceFreshnessHeader;
 
-  RunTransactionTest(cache.http_cache(), stale_while_revalidate_transaction);
+  RunFixtureTransactionTest();
 
-  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(2, transaction_count());
 }
 
 static void CheckResourceFreshnessAbsent(const net::HttpRequestInfo* request,
@@ -6768,25 +6801,416 @@ static void CheckResourceFreshnessAbsent(const net::HttpRequestInfo* request,
 
 // Verify that the Resource-Freshness header is not sent when
 // stale-while-revalidate is 0.
-TEST(HttpCache, ResourceFreshnessHeaderNotSent) {
-  MockHttpCache cache;
-
-  ScopedMockTransaction stale_while_revalidate_transaction(
-      kSimpleGET_Transaction);
-  stale_while_revalidate_transaction.response_headers =
-      "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
-      "Age: 10801\n"
-      "Cache-Control: max-age=3600,stale-while-revalidate=0\n";
+TEST_F(HttpCacheStaleWhileRevalidateTest, ResourceFreshnessHeaderNotSent) {
+  age_ = 10801;
+  stale_while_revalidate_ = 0;
 
   // Write to the cache.
-  RunTransactionTest(cache.http_cache(), stale_while_revalidate_transaction);
+  RunFixtureTransactionTest();
 
-  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, transaction_count());
 
   // Send the request again and check that Resource-Freshness header is absent.
-  stale_while_revalidate_transaction.handler = CheckResourceFreshnessAbsent;
+  transaction_.handler = CheckResourceFreshnessAbsent;
 
-  RunTransactionTest(cache.http_cache(), stale_while_revalidate_transaction);
+  RunFixtureTransactionTest();
 
-  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(2, transaction_count());
+}
+
+// Verify that when stale-while-revalidate applies the response is read from
+// cache.
+TEST_F(HttpCacheStaleWhileRevalidateTest, ReadFromCache) {
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Let the async request complete.
+  RunLoop().RunUntilIdle();

[rvargas (doing something else), 2014/09/17 04:32:19]

What happens if this line is not here? (being the last one on the test) Maybe we
should remove these?

[Adam Rice, 2014/09/30 13:44:50]

I was concerned about the callbacks interfering with other tests, but since they
only hold a weak pointer to a deleted HttpCache there isn't any real danger. I
have removed this and similar lines.

+}
+
+// Verify that when stale-while-revalidate applies an asynchronous request is
+// sent.
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncRequestSent) {
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Let the async request execute.
+  RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, transaction_count());
+}
+
+// Verify that tearing down the HttpCache with an async revalidation in progress
+// does not break anything (this test is most likely to find problems when run
+// with a memory checker such as AddressSanitizer).
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncTearDown) {
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+}
+
+static void CheckIfModifiedSinceHeader(const net::HttpRequestInfo* request,
+                                       std::string* response_status,
+                                       std::string* response_headers,
+                                       std::string* response_data) {
+  std::string value;
+  EXPECT_TRUE(request->extra_headers.GetHeader("If-Modified-Since", &value));
+  EXPECT_EQ("Sat, 18 Apr 2007 01:10:43 GMT", value);
+}
+
+// Verify that the async revalidation contains an If-Modified-Since header.
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncRequestIfModifiedSince) {
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  transaction_.handler = CheckIfModifiedSinceHeader;
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  RunLoop().RunUntilIdle();
+}
+
+static void CheckIfNoneMatchHeader(const net::HttpRequestInfo* request,
+                                   std::string* response_status,
+                                   std::string* response_headers,
+                                   std::string* response_data) {
+  std::string value;
+  EXPECT_TRUE(request->extra_headers.GetHeader("If-None-Match", &value));
+  EXPECT_EQ("\"40a1-1320-4f6adefa22a40\"", value);
+}
+
+// If the response had ETag rather than Last-Modified, then that is used to
+// conditionalise the response.
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncRequestIfNoneMatch) {
+  validator_ = "Etag: \"40a1-1320-4f6adefa22a40\"";
+
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  transaction_.handler = CheckIfNoneMatchHeader;
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  RunLoop().RunUntilIdle();
+}
+
+static void CheckResourceFreshnessHeaderPresent(
+    const net::HttpRequestInfo* request,
+    std::string* response_status,
+    std::string* response_headers,
+    std::string* response_data) {
+  EXPECT_TRUE(request->extra_headers.HasHeader("Resource-Freshness"));
+}
+
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncRequestHasResourceFreshness) {
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  transaction_.handler = CheckResourceFreshnessHeaderPresent;
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  RunLoop().RunUntilIdle();
+}
+
+// Verify that when age > max-age + stale-while-revalidate stale results are
+// not returned.
+TEST_F(HttpCacheStaleWhileRevalidateTest, NotAppliedIfTooStale) {
+  age_ = 10801;
+
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Reading back reads from the network.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, open_count());
+  EXPECT_EQ(2, transaction_count());
+}
+
+// HEAD requests should be able to take advantage of stale-while-revalidate.
+TEST_F(HttpCacheStaleWhileRevalidateTest, WorksForHeadMethod) {
+  // Write to the cache. This has to be a GET request; HEAD requests don't
+  // create new cache entries (as of August 2014).

[rvargas (doing something else), 2014/09/17 04:32:20]

nit: remove the date?

[Adam Rice, 2014/09/30 13:44:50]

Done.

+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Read back from the cache, and trigger an asynchronous HEAD request.
+  transaction_.method = "HEAD";
+  transaction_.data = "";
+
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Let the network request proceed.
+  RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2, transaction_count());
+}
+
+// POST requests should not use stale-while-revalidate.
+TEST_F(HttpCacheStaleWhileRevalidateTest, NotAppliedToPost) {
+  transaction_ = ScopedMockTransaction(kSimplePOST_Transaction);
+
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, open_count());
+  EXPECT_EQ(1, transaction_count());
+
+  // Reading back reads from the network.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, open_count());
+  EXPECT_EQ(2, transaction_count());
+}
+
+// Async revalidation is issued to the original URL.
+TEST_F(HttpCacheStaleWhileRevalidateTest, AsyncRequestUrlMatches) {
+  net::CapturingBoundNetLog log;
+  net::LoadTimingInfo load_timing_info;
+
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  // Read back from the cache.
+  RunTransactionTestAndGetTiming(
+      cache_.http_cache(), transaction_, log.bound(), &load_timing_info);
+
+  // Let the async request execute.
+  RunLoop().RunUntilIdle();
+
+  net::CapturingNetLog::CapturedEntryList entries;

[rvargas (doing something else), 2014/09/17 04:32:19]

using a handler seems better to me (ie, looking at the server side). Unless this
test is meant to verify the log...

[Adam Rice, 2014/09/30 13:44:50]

Done.

+  log.GetEntries(&entries);
+  size_t index =
+      net::ExpectLogContainsSomewhere(entries,
+                                      0,
+                                      net::NetLog::TYPE_ASYNC_REVALIDATION,
+                                      net::NetLog::PHASE_BEGIN);
+  ASSERT_LT(index, entries.size());
+  std::string url_value;
+  EXPECT_TRUE(entries[index].GetStringValue("url", &url_value));
+  EXPECT_EQ(transaction_.url, url_value);
+}
+
+class SyncLoadFlagTest : public HttpCacheStaleWhileRevalidateTest,
+                         public ::testing::WithParamInterface<int> {};
+
+// Flags which should always cause the request to be synchronous.
+TEST_P(SyncLoadFlagTest, MustBeSynchronous) {
+  transaction_.load_flags |= GetParam();
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Reading back reads from the network.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(2, transaction_count());
+}
+
+INSTANTIATE_TEST_CASE_P(HttpCacheStaleWhileRevalidate,
+                        SyncLoadFlagTest,
+                        ::testing::Values(net::LOAD_VALIDATE_CACHE,
+                                          net::LOAD_BYPASS_CACHE,
+                                          net::LOAD_DISABLE_CACHE));
+
+TEST_F(HttpCacheStaleWhileRevalidateTest,
+       PreferringCacheDoesNotTriggerAsyncRequest) {
+  transaction_.load_flags |= net::LOAD_PREFERRING_CACHE;
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Reading back reads from the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // If there was an async transaction created, it would run now.
+  RunLoop().RunUntilIdle();
+
+  // There was no async transaction.
+  EXPECT_EQ(1, transaction_count());
+}
+
+TEST_F(HttpCacheStaleWhileRevalidateTest, NotUsedWhenDisabled) {
+  cache_.http_cache()->set_use_stale_while_revalidate_for_testing(false);
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // A synchronous revalidation is performed.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(2, transaction_count());
+}
+
+TEST_F(HttpCacheStaleWhileRevalidateTest,
+       OnlyFromCacheDoesNotTriggerAsyncRequest) {
+  transaction_.load_flags |= net::LOAD_ONLY_FROM_CACHE;
+  transaction_.return_code = net::ERR_CACHE_MISS;
+
+  // Writing to the cache should fail, because we are avoiding the network.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(0, transaction_count());
+
+  RunLoop().RunUntilIdle();
+
+  // Still nothing.
+  EXPECT_EQ(0, transaction_count());
+}
+
+// A certificate error during an asynchronous fetch should cause the next fetch
+// to proceed synchronously.
+TEST_F(HttpCacheStaleWhileRevalidateTest, CertificateErrorCausesRefetch) {

[rvargas (doing something else), 2014/09/17 04:32:19]

hmmm. Is this testing for... an incomplete implementation? This is supposed to
be fixed the next CL, right?

[Adam Rice, 2014/09/30 13:44:50]

Yes. I am just verifying the current behaviour. I have added a TODO indicating
that the behaviour will change.

+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Now read back. RunTransactionTestBase() expects to receive the network
+  // error back from the HttpCache::Transaction, but since the cache request
+  // will return OK we need to duplicate some of its implementation here.
+  transaction_.return_code = net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+  net::TestCompletionCallback callback;
+  scoped_ptr<net::HttpTransaction> trans;
+  int rv =
+      cache_.http_cache()->CreateTransaction(net::DEFAULT_PRIORITY, &trans);
+  EXPECT_EQ(net::OK, rv);
+  ASSERT_TRUE(trans.get());
+
+  MockHttpRequest request(transaction_);
+  rv = trans->Start(&request, callback.callback(), net::BoundNetLog());
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+  ASSERT_EQ(net::OK, callback.WaitForResult());
+  ReadAndVerifyTransaction(trans.get(), transaction_);
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Allow the asynchronous fetch to run.
+  RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(2, transaction_count());
+
+  // Now run the transaction again. It should run synchronously.
+  transaction_.return_code = net::OK;
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(3, transaction_count());
+}
+
+// Ensure that the response cached by the asynchronous request is not truncated,
+// even if the server is slow.
+TEST_F(HttpCacheStaleWhileRevalidateTest, EntireResponseCached) {
+  transaction_.test_mode = TEST_MODE_SLOW_READ;
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  // Let the async request execute.
+  RunLoop().RunUntilIdle();
+
+  // The cache entry should still be complete.
+  transaction_.load_flags = net::LOAD_ONLY_FROM_CACHE;
+  RunFixtureTransactionTest();
+}
+
+// Verify that there are no race conditions in the completely synchronous case.
+TEST_F(HttpCacheStaleWhileRevalidateTest, SynchronousCaseWorks) {
+  transaction_.test_mode = TEST_MODE_SYNC_ALL;
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Let the async request execute.
+  RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, transaction_count());
+}
+
+static void CheckLoadFlagsAsyncRevalidation(const net::HttpRequestInfo* request,
+                                            std::string* response_status,
+                                            std::string* response_headers,
+                                            std::string* response_data) {
+  EXPECT_EQ(net::LOAD_ASYNC_REVALIDATION, request->load_flags);
+}
+
+// Check that the load flags on the async request are the same as the load flags
+// on the original request, plus LOAD_ASYNC_REVALIDATION.
+TEST_F(HttpCacheStaleWhileRevalidateTest, LoadFlagsAsyncRevalidation) {
+  transaction_.load_flags = net::LOAD_NORMAL;
+  // Write to the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  // Read back from the cache.
+  RunFixtureTransactionTest();
+
+  EXPECT_EQ(1, transaction_count());
+
+  transaction_.handler = CheckLoadFlagsAsyncRevalidation;
+  // Let the async request execute.
+  RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, transaction_count());
+}
+
+// Cache-Control: must-revalidate directive should override the
+// stale-while-revalidate directive and cause a synchronous revalidation.
+TEST_F(HttpCacheStaleWhileRevalidateTest, MustRevalidateOverrides) {

[rvargas (doing something else), 2014/09/17 04:32:19]

This seems like a test that belongs to response headers instead.

[Adam Rice, 2014/09/30 13:44:50]

Yes. Thank you. I have moved it there.

+  transaction_.response_headers =
+      "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT"
+      "Age: 3601\n"
+      "Cache-Control: "
+      "max-age=3600,stale-while-revalidate=3600,must-revalidate\n";
+
+  // Write to the cache. This test uses custom response headers, so it cannot
+  // use RunFixtureTransactionTest().
+  RunTransactionTest(cache_.http_cache(), transaction_);
+
+  EXPECT_EQ(1, transaction_count());
+
+  // A synchronous revalidation is performed.
+  RunTransactionTest(cache_.http_cache(), transaction_);
+
+  EXPECT_EQ(2, transaction_count());
 }