Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

net/http/http_cache_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_cache.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 1751 matching lines @@
   // The second request is queued after the first one.
 
   c2.callback.WaitForResult();
   ReadAndVerifyTransaction(c2.trans.get(), kSimpleGET_Transaction);
 
   // Complete the first transaction.
   c1.callback.WaitForResult();
   ReadAndVerifyTransaction(c1.trans.get(), kSimpleGET_Transaction);
 }
 
+// Tests that a (simulated) timeout allows transactions waiting on the cache
+// lock to continue but read only transactions to error out.
+TEST(HttpCache, SimpleGET_WriterTimeoutReadOnlyError) {
+  MockHttpCache cache;
+
+  // Simulate timeout.
+  cache.BypassCacheLock();

[asanka, 2017/02/07 22:56:03]

This method is poorly named :-(

[shivanisha, 2017/02/08 19:14:13]

Updated to SimulateCacheLockTimeout for clarity and removed the comment over it.

+
+  MockHttpRequest request(kSimpleGET_Transaction);
+  Context c1, c2;
+  ASSERT_THAT(cache.CreateTransaction(&c1.trans), IsOk());
+  ASSERT_EQ(ERR_IO_PENDING, c1.trans->Start(&request, c1.callback.callback(),
+                                            NetLogWithSource()));
+
+  // Using load flags similar to MetadataWriter.

[asanka, 2017/02/07 22:56:03]

There are two things to test:

1) That HttpCache::WriteMetadata() will fail if the cache lock times out.
2) That HttpCache::Transaction fails if LOAD_ONLY_FROM_CACHE is set and the
cache lock times out.

Due to the additional flags set by WriteMetadata, it's not robust to assume that
#1 implies #2. But the inverse is defensible. So let's change this test to only
specify LOAD_ONLY_FROM_CACHE. The other flags should be moot.

To close the loop I'd suggest adding a test case to the WriteMetadata_* group
above which simulates the cache lock timeout while invoking
HttpCache::WriteMetadata().

[shivanisha, 2017/02/08 19:14:13]

Done.

The new test added for WriteMetadata_* group cannot test 2 simultaneous metadata
writers because of an issue in the way MetadataWriter uses the load flags. I
have added a todo note and created a bug for the same. In the test I have added
one normal (non read only) transaction and one MetadataWriter to achieve cache
lock timeout. 

+  request.load_flags =
+      LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION | LOAD_SKIP_VARY_CHECK;
+  ASSERT_THAT(cache.CreateTransaction(&c2.trans), IsOk());
+  ASSERT_EQ(ERR_IO_PENDING, c2.trans->Start(&request, c2.callback.callback(),
+                                            NetLogWithSource()));
+
+  // The second request is queued after the first one.
+  int res = c2.callback.WaitForResult();
+  ASSERT_EQ(ERR_CACHE_MISS, res);
+
+  // Complete the first transaction.
+  c1.callback.WaitForResult();
+  ReadAndVerifyTransaction(c1.trans.get(), kSimpleGET_Transaction);
+}
+
 TEST(HttpCache, SimpleGET_AbandonedCacheRead) {
   MockHttpCache cache;
 
   // write to the cache
   RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
 
   MockHttpRequest request(kSimpleGET_Transaction);
   TestCompletionCallback callback;
 
   std::unique_ptr<HttpTransaction> trans;
@@ skipping 6472 matching lines @@
   RunTransactionTestWithResponseInfo(cache.http_cache(),
                                      kTypicalGET_Transaction, &response_info);
 
   EXPECT_FALSE(response_info.was_cached);
   EXPECT_TRUE(response_info.network_accessed);
   EXPECT_EQ(CacheEntryStatus::ENTRY_CANT_CONDITIONALIZE,
             response_info.cache_entry_status);
 }
 
 }  // namespace net