HttpCache::Transaction layer allowing parallel validation

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 71509a421e268a550215c7c941993351dd15254b..6102d65691d6171484da2f1f61569186b23073ef 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -1376,7 +1376,8 @@ TEST(HttpCache, SimpleGET_ManyReaders) {
   base::RunLoop().RunUntilIdle();
 
   // The first request should be a writer at this point, and the subsequent
-  // requests should be pending.
+  // requests should have passed the validation phase and waiting for the
+  // response to be written to the cache before they can read.
 
   EXPECT_EQ(1, cache.network_layer()->transaction_count());
   EXPECT_EQ(0, cache.disk_cache()->open_count());
@@ -1408,6 +1409,86 @@ TEST(HttpCache, SimpleGET_ManyReaders) {
   }
 }
 
+// Tests parallel validation similar to the above test, except here validation
+// request is sent out to the network and results in 304.
+TEST(HttpCache, SimpleGET_ParallelValidation) {
+  MockHttpCache cache;
+
+  std::vector<MockHttpRequest> request;
+  ScopedMockTransaction transaction(kTypicalGET_Transaction);
+  transaction.response_headers =
+      "Etag: \"foopy\"\n"
+      "Cache-Control: max-age=0\n";
+  MockHttpRequest request0(transaction);
+  request.push_back(request0);
+
+  ScopedMockTransaction transaction1(kTypicalGET_Transaction);
+  transaction1.status = "HTTP/1.1 304 Not Modified";
+  transaction1.load_flags |= LOAD_VALIDATE_CACHE;
+  transaction1.response_headers =
+      "Etag: \"foopy\"\n"
+      "Cache-Control: max-age=3600\n";
+  MockHttpRequest request1(transaction1);
+  request.push_back(request1);
+
+  std::vector<Context*> context_list;
+  const int kNumTransactions = 2;
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    context_list.push_back(new Context());
+    Context* c = context_list[i];
+
+    c->result = cache.CreateTransaction(&c->trans);
+    ASSERT_THAT(c->result, IsOk());
+    EXPECT_EQ(LOAD_STATE_IDLE, c->trans->GetLoadState());
+
+    c->result = c->trans->Start(&request[i], c->callback.callback(),
+                                NetLogWithSource());
+  }
+
+  // All requests are waiting for the active entry.
+  for (int i = 0; i < kNumTransactions; ++i) {
+    Context* c = context_list[i];
+    EXPECT_EQ(LOAD_STATE_WAITING_FOR_CACHE, c->trans->GetLoadState());
+  }
+
+  // Allow all requests to move from the Create queue to the active entry.
+  base::RunLoop().RunUntilIdle();
+
+  // The first request should be a writer at this point, and the subsequent
+  // requests should have passed the validation phase and waiting for the
+  // response to be written to the cache before they can read.
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // All requests depend on the writer, and the writer is between Start and
+  // Read, i.e. idle.
+  for (int i = 0; i < kNumTransactions; ++i) {
+    Context* c = context_list[i];
+    EXPECT_EQ(LOAD_STATE_IDLE, c->trans->GetLoadState());
+  }
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    Context* c = context_list[i];
+    if (c->result == ERR_IO_PENDING)
+      c->result = c->callback.WaitForResult();
+    ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
+  }
+
+  // We should not have had to re-open the disk entry
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    Context* c = context_list[i];
+    delete c;
+  }
+}
+
 // This is a test for http://code.google.com/p/chromium/issues/detail?id=4769.
 // If cancelling a request is racing with another request for the same resource
 // finishing, we have to make sure that we remove both transactions from the
@@ -1453,7 +1534,6 @@ TEST(HttpCache, SimpleGET_RacingReaders) {
   ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
 
   // Now we have 2 active readers and two queued transactions.
-
   EXPECT_EQ(LOAD_STATE_IDLE, context_list[2]->trans->GetLoadState());
   EXPECT_EQ(LOAD_STATE_WAITING_FOR_CACHE,
             context_list[3]->trans->GetLoadState());
@@ -1568,11 +1648,12 @@ TEST(HttpCache, FastNoStoreGET_DoneWithPending) {
   base::RunLoop().RunUntilIdle();
 
   // The first request should be a writer at this point, and the subsequent
-  // requests should be pending.
+  // requests should have completed validation. Since the validation does not
+  // result in a match, a new entry would be created.
 
-  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
   EXPECT_EQ(0, cache.disk_cache()->open_count());
-  EXPECT_EQ(1, cache.disk_cache()->create_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
 
   // Now, make sure that the second request asks for the entry not to be stored.
   request_handler.set_no_store(true);
@@ -2541,6 +2622,8 @@ TEST(HttpCache, ETagGET_ConditionalRequest_304_NoStore) {
 //     be returned.
 // (4) loads |kUrl| from cache only -- expects |cached_response_2| to be
 //     returned.
+// The entry will be created once and will be opened for the 3 subsequent
+// requests.
 static void ConditionalizedRequestUpdatesCacheHelper(
     const Response& net_response_1,
     const Response& net_response_2,
@@ -6846,46 +6929,6 @@ TEST(HttpCache, WriteMetadata_Fail) {
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
-// Tests that if a metadata writer transaction hits cache lock timeout, it will

[shivanisha, 2017/03/07 12:51:32]

This is a test from an unrelated CL https://codereview.chromium.org/2671793002.
Removed it since this test doesn't really test what it is supposed to. Reason
being that MetadataWriter does not use CreateTransaction but calls "new"
directly and thus does not fill the bypass_lock_for_test_ field.

-// error out.
-TEST(HttpCache, WriteMetadata_CacheLockTimeout) {
-  MockHttpCache cache;
-
-  // Write to the cache
-  HttpResponseInfo response;
-  RunTransactionTestWithResponseInfo(cache.http_cache(), kSimpleGET_Transaction,
-                                     &response);
-  EXPECT_FALSE(response.metadata.get());
-
-  MockHttpRequest request(kSimpleGET_Transaction);
-  Context c1;
-  ASSERT_THAT(cache.CreateTransaction(&c1.trans), IsOk());
-  ASSERT_EQ(ERR_IO_PENDING, c1.trans->Start(&request, c1.callback.callback(),
-                                            NetLogWithSource()));
-
-  cache.SimulateCacheLockTimeout();
-
-  // Write meta data to the same entry.
-  scoped_refptr<IOBufferWithSize> buf(new IOBufferWithSize(50));
-  memset(buf->data(), 0, buf->size());
-  base::strlcpy(buf->data(), "Hi there", buf->size());
-  cache.http_cache()->WriteMetadata(GURL(kSimpleGET_Transaction.url),
-                                    DEFAULT_PRIORITY, response.response_time,
-                                    buf.get(), buf->size());
-
-  // Release the buffer before the operation takes place.
-  buf = NULL;
-
-  // Makes sure we finish pending operations.
-  base::RunLoop().RunUntilIdle();
-
-  RunTransactionTestWithResponseInfo(cache.http_cache(), kSimpleGET_Transaction,
-                                     &response);
-
-  // The writer transaction should fail due to cache lock timeout.
-  ASSERT_FALSE(response.metadata.get());
-}
-
 // Tests that we ignore VARY checks when writing metadata since the request
 // headers for the WriteMetadata transaction are made up.
 TEST(HttpCache, WriteMetadata_IgnoreVary) {