Disk cache: Remove all non essential synchronization from the cache destructor.

net/disk_cache/backend_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 "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/platform_thread.h"
@@ skipping 17 matching lines @@
 
 using base::Time;
 
 static const int kDiskDelayMs = 20;
 
 // Tests that can run with different types of caches.
 class DiskCacheBackendTest : public DiskCacheTestWithCache {
  protected:
   void BackendBasics();
   void BackendKeying();
+  void BackendShutdownWithPendingIO(bool wait);
+  void BackendShutdownWithPendingIO2(bool wait);
+  void BackendShutdownWithPendingIO3(bool wait);
   void BackendSetSize();
   void BackendLoad();
   void BackendChain();
   void BackendValidEntry();
   void BackendInvalidEntry();
   void BackendInvalidEntryRead();
   void BackendInvalidEntryWithLoad();
   void BackendTrimInvalidEntry();
   void BackendTrimInvalidEntry2();
   void BackendEnumerations();
@@ skipping 220 matching lines @@
   ASSERT_EQ(0, WriteData(entry, 0, 20000, buffer1, 0, false));
   entry->Close();
 
   // And verify that the first file is still there.
   scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize));
   ASSERT_EQ(kSize, file_util::ReadFile(filename, buffer2->data(), kSize));
   EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), kSize));
 }
 
 // Tests that we deal with file-level pending operations at destruction time.
-TEST_F(DiskCacheTest, ShutdownWithPendingIO) {
+void DiskCacheBackendTest::BackendShutdownWithPendingIO(bool wait) {

[gavinp, 2012/03/18 00:35:26]

A single bool argument at a call site can be confusing to read.  How about we
declare an enum?

enum PendingIOOptions {
  WAIT_FOR_PENDING_IO,
  DROP_PENDING_IO,
};

[gavinp, 2012/03/18 00:35:26]

How about calling this BackendShutdownWithPendingCreate() ?

   net::TestCompletionCallback cb;
 
   {
     ASSERT_TRUE(CleanupCacheDir());
     base::Thread cache_thread("CacheThread");
     ASSERT_TRUE(cache_thread.StartWithOptions(
                     base::Thread::Options(MessageLoop::TYPE_IO, 0)));
 
     disk_cache::Backend* cache;
+    uint32 flags = wait ? disk_cache::kNoRandom : disk_cache::kNone;

[gavinp, 2012/03/18 00:35:26]

uint32 flags = disk_cache::kNoBuffering;
if (pending_io_option == WAIT_FOR_PENDING_IO)
  flags |= disk_cache::kNoRandom;

+    flags |= disk_cache::kNoBuffering;
     int rv = disk_cache::BackendImpl::CreateBackend(
-                 cache_path_, false, 0, net::DISK_CACHE, disk_cache::kNoRandom,
+                 cache_path_, false, 0, net::DISK_CACHE, flags,
                  base::MessageLoopProxy::current(), NULL,
                  &cache, cb.callback());
     ASSERT_EQ(net::OK, cb.GetResult(rv));
 
     disk_cache::EntryImpl* entry;
     rv = cache->CreateEntry(
         "some key", reinterpret_cast<disk_cache::Entry**>(&entry),
         cb.callback());
     ASSERT_EQ(net::OK, cb.GetResult(rv));
 
@@ skipping 12 matching lines @@
     }
 
     // Don't call Close() to avoid going through the queue or we'll deadlock
     // waiting for the operation to finish.
     entry->Release();
 
     // The cache destructor will see one pending operation here.
     delete cache;
 
     if (rv == net::ERR_IO_PENDING) {
-      EXPECT_TRUE(cb.have_result());
+      if (wait) {

[gavinp, 2012/03/18 00:35:26]

Take it or leave it nit: I favour no braces in this case, the vertical
compaction make the relationship easier to read to my eyes

+        EXPECT_TRUE(cb.have_result());
+      } else {
+        EXPECT_FALSE(cb.have_result());
+      }
     }
   }
 
   MessageLoop::current()->RunAllPending();
 }
 
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO) {
+  // Do full synchronization at destruction.
+  BackendShutdownWithPendingIO(true);
+}
+
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO_Fast) {
+  // The integrity test sets kNoRandom so there's a version mismatch if we don't
+  // force new eviction.
+  SetNewEviction();
+  // Do minimal synchronization at destruction.
+  BackendShutdownWithPendingIO(false);
+}
+
 // Tests that we deal with background-thread pending operations.
-TEST_F(DiskCacheTest, ShutdownWithPendingIO2) {
+void DiskCacheBackendTest::BackendShutdownWithPendingIO2(bool wait) {

[gavinp, 2012/03/18 00:35:26]

How about BackendShutdownWithPendingWrites() ???

   net::TestCompletionCallback cb;
 
   {
     ASSERT_TRUE(CleanupCacheDir());
     base::Thread cache_thread("CacheThread");
     ASSERT_TRUE(cache_thread.StartWithOptions(
                     base::Thread::Options(MessageLoop::TYPE_IO, 0)));
 
     disk_cache::Backend* cache;
+    uint32 flags = wait ? disk_cache::kNoRandom : disk_cache::kNone;
+    flags |= disk_cache::kNoBuffering;
     int rv = disk_cache::BackendImpl::CreateBackend(
-        cache_path_, false, 0, net::DISK_CACHE, disk_cache::kNoRandom,
+        cache_path_, false, 0, net::DISK_CACHE, flags,
         cache_thread.message_loop_proxy(), NULL, &cache, cb.callback());
     ASSERT_EQ(net::OK, cb.GetResult(rv));
 
     disk_cache::Entry* entry;
     rv = cache->CreateEntry("some key", &entry, cb.callback());
     ASSERT_EQ(net::OK, cb.GetResult(rv));
 
     const int kSize = 25000;
     scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kSize));
     CacheTestFillBuffer(buffer->data(), kSize, false);
 
     rv = entry->WriteData(0, 0, buffer, kSize, cb.callback(), false);
     EXPECT_EQ(net::ERR_IO_PENDING, rv);
 
     entry->Close();
 
     // The cache destructor will see two pending operations here.
     delete cache;
   }
 
   MessageLoop::current()->RunAllPending();
 }
 
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO2) {
+  // Do full synchronization at destruction.
+  BackendShutdownWithPendingIO2(true);
+}
+
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO2_Fast) {
+  // The integrity test sets kNoRandom so there's a version mismatch if we don't
+  // force new eviction.
+  SetNewEviction();
+  // Do minimal synchronization at destruction.
+  BackendShutdownWithPendingIO2(false);
+}
+
+// Tests that we deal with create-type pending operations.
+void DiskCacheBackendTest::BackendShutdownWithPendingIO3(bool wait) {

[gavinp, 2012/03/18 00:35:26]

BackendShutdownWithPendingBufferedCreates ?

+  net::TestCompletionCallback cb;
+
+  {
+    ASSERT_TRUE(CleanupCacheDir());
+    base::Thread cache_thread("CacheThread");
+    ASSERT_TRUE(cache_thread.StartWithOptions(
+                    base::Thread::Options(MessageLoop::TYPE_IO, 0)));
+
+    disk_cache::Backend* cache;
+    disk_cache::BackendFlags flags =
+        wait ? disk_cache::kNoRandom : disk_cache::kNone;
+    int rv = disk_cache::BackendImpl::CreateBackend(
+        cache_path_, false, 0, net::DISK_CACHE, flags,
+        cache_thread.message_loop_proxy(), NULL, &cache, cb.callback());
+    ASSERT_EQ(net::OK, cb.GetResult(rv));
+
+    disk_cache::Entry* entry;
+    rv = cache->CreateEntry("some key", &entry, cb.callback());
+    ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+    delete cache;
+    EXPECT_FALSE(cb.have_result());
+  }
+
+  MessageLoop::current()->RunAllPending();
+}
+
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO3) {
+  // Do full synchronization at destruction.
+  BackendShutdownWithPendingIO3(true);
+}
+
+// We'll be leaking an entry from this test.
+TEST_F(DiskCacheBackendTest, ShutdownWithPendingIO3_Fast) {
+  // The integrity test sets kNoRandom so there's a version mismatch if we don't
+  // force new eviction.
+  SetNewEviction();
+  // Do minimal synchronization at destruction.
+  BackendShutdownWithPendingIO3(false);
+}
+
 TEST_F(DiskCacheTest, TruncatedIndex) {
   ASSERT_TRUE(CleanupCacheDir());
   FilePath index = cache_path_.AppendASCII("index");
   ASSERT_EQ(5, file_util::WriteFile(index, "hello", 5));
 
   base::Thread cache_thread("CacheThread");
   ASSERT_TRUE(cache_thread.StartWithOptions(
                   base::Thread::Options(MessageLoop::TYPE_IO, 0)));
   net::TestCompletionCallback cb;
 
@@ skipping 2115 matching lines @@
   // Ping the oldest entry.
   cache_->OnExternalCacheHit("key0");
 
   TrimForTest(false);
 
   // Make sure the older key remains.
   EXPECT_EQ(1, cache_->GetEntryCount());
   ASSERT_EQ(net::OK, OpenEntry("key0", &entry));
   entry->Close();
 }