Disk cache: Switch the disk cache to use the cache_thread. ...

net/disk_cache/backend_unittest.cc

Index: net/disk_cache/backend_unittest.cc
===================================================================
--- net/disk_cache/backend_unittest.cc	(revision 51470)
+++ net/disk_cache/backend_unittest.cc	(working copy)
@@ -7,7 +7,6 @@
 #include "base/path_service.h"
 #include "base/platform_thread.h"
 #include "base/string_util.h"
-#include "base/thread.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
@@ -254,8 +253,9 @@
   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) {
-  TestCompletionCallback callback;
+  TestCompletionCallback cb;
 
   {
     FilePath path = GetCacheFilePath();
@@ -267,33 +267,80 @@
     disk_cache::Backend* cache;
     int rv = disk_cache::BackendImpl::CreateBackend(
                  path, false, 0, net::DISK_CACHE, disk_cache::kNoRandom,
-                 cache_thread.message_loop_proxy(), &cache, &callback);
-    ASSERT_EQ(net::OK, callback.GetResult(rv));
+                 base::MessageLoopProxy::CreateForCurrentThread(), &cache, &cb);
+    ASSERT_EQ(net::OK, cb.GetResult(rv));
 
-    disk_cache::Entry* entry;
-    rv = cache->CreateEntry("some key", &entry, &callback);
-    ASSERT_EQ(net::OK, callback.GetResult(rv));
+    disk_cache::EntryImpl* entry;
+    rv = cache->CreateEntry("some key",
+                            reinterpret_cast<disk_cache::Entry**>(&entry), &cb);
+    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);
 
     for (int i = 0; i < 10 * 1024 * 1024; i += 64 * 1024) {
-      int rv = entry->WriteData(0, i, buffer, kSize, &callback, false);
+      // We are using the current thread as the cache thread because we want to
+      // be able to call directly this method to make sure that the OS (instead
+      // of us switching thread) is returning IO pending.
+      rv = entry->WriteDataImpl(0, i, buffer, kSize, &cb, false);
       if (rv == net::ERR_IO_PENDING)
         break;
       EXPECT_EQ(kSize, rv);
     }
 
-    entry->Close();
+    // 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());
+    }
   }
 
   MessageLoop::current()->RunAllPending();
 }
 
+// Tests that we deal with background-thread pending operations.
+TEST_F(DiskCacheTest, ShutdownWithPendingIO2) {
+  TestCompletionCallback cb;
+
+  {
+    FilePath path = GetCacheFilePath();
+    ASSERT_TRUE(DeleteCache(path));
+    base::Thread cache_thread("CacheThread");
+    ASSERT_TRUE(cache_thread.StartWithOptions(
+                    base::Thread::Options(MessageLoop::TYPE_IO, 0)));
+
+    disk_cache::Backend* cache;
+    int rv = disk_cache::BackendImpl::CreateBackend(
+                 path, false, 0, net::DISK_CACHE, disk_cache::kNoRandom,
+                 cache_thread.message_loop_proxy(), &cache, &cb);
+    ASSERT_EQ(net::OK, cb.GetResult(rv));
+
+    disk_cache::Entry* entry;
+    rv = cache->CreateEntry("some key", &entry, &cb);
+    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, 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(DiskCacheTest, TruncatedIndex) {
   FilePath path = GetCacheFilePath();
   ASSERT_TRUE(DeleteCache(path));
@@ -344,6 +391,7 @@
   EXPECT_EQ(cache_size * 3 / 4, entry->WriteData(0, 0, buffer,
                 cache_size * 3 / 4, NULL, false));
   entry->Close();
+  FlushQueueForTest();
 
   SetMaxSize(cache_size);
 
@@ -354,10 +402,12 @@
                                               NULL, false)) << "trim the cache";
   entry->Close();
 
-  EXPECT_NE(net::OK, OpenEntry(first, &entry));
+  // On a slow system we may end up evicting everything not currently in use.

[gavinp, 2010/07/08 02:18:14]

Does reordering the assert for OpenEntry(second) and OpenEntry(first) make a
difference?  Why?

[rvargas (doing something else), 2010/07/08 17:30:50]

The problem with the old code is that it tests that only one entry (the oldest
one) is evicted. Eviction actually happens when the max_size limit is reached,
when an entry is charged by the space. In this case, it happens when "second" is
flushed from it's destructor... in other words, when the posted Close() executes
on the cache thread.

The issue with a slow system (valgrind) is that eviction of a single entry takes
more than 20 ms so we assume that we must keep working on that, and we post a
task to continue the eviction process... and given that we now have two threads,
that second task will actually run and evict the second entry so we'll fail to
open it.

By opening "second" right away, we don't give the eviction code a chance to
evict it, because when that posted task to evict runs, the entry is in use and
will not be evicted.

The sad thing is that now that we have two threads (and control for only one),
there are a lot of tests that are quite delicate in the order of events needed
to trigger the behavior under test :(

   ASSERT_EQ(net::OK, OpenEntry(second, &entry));
   EXPECT_EQ(cache_size / 10, entry->GetDataSize(0));
   entry->Close();
+
+  EXPECT_NE(net::OK, OpenEntry(first, &entry));
 }
 
 TEST_F(DiskCacheBackendTest, SetSize) {
@@ -402,6 +452,7 @@
     entries[i]->Doom();
     entries[i]->Close();
   }
+  FlushQueueForTest();
   EXPECT_EQ(0, cache_->GetEntryCount());
 }
 
@@ -625,6 +676,7 @@
   EXPECT_EQ(2, cache_->GetEntryCount());
   SetMaxSize(kSize);
   entry->Close();  // Trim the cache.
+  FlushQueueForTest();
 
   // If we evicted the entry in less than 20mS, we have one entry in the cache;
   // if it took more than that, we posted a task and we'll delete the second
@@ -685,6 +737,7 @@
   }
 
   entry->Close();  // Trim the cache.
+  FlushQueueForTest();
 
   // We may abort the eviction before cleaning up everything.
   MessageLoop::current()->RunAllPending();
@@ -1259,7 +1312,7 @@
   EXPECT_EQ(2, cache_->GetEntryCount());
 
   EXPECT_NE(net::OK, OpenNextEntry(&iter, &entry));
-  MessageLoop::current()->RunAllPending();
+  FlushQueueForTest();  // Allow the restart to finish.
   EXPECT_EQ(0, cache_->GetEntryCount());
 }
 
@@ -1310,7 +1363,8 @@
   EXPECT_NE(net::OK, CreateEntry("Something new", &entry2));
 
   entry1->Close();
-  MessageLoop::current()->RunAllPending();
+  FlushQueueForTest();  // Flushing the Close posts a task to restart the cache.
+  FlushQueueForTest();  // This one actually allows that task to complete.
 
   EXPECT_EQ(0, cache_->GetEntryCount());
 }
@@ -1365,7 +1419,7 @@
     ASSERT_LT(count, 9);
   };
 
-  MessageLoop::current()->RunAllPending();
+  FlushQueueForTest();
   EXPECT_EQ(0, cache_->GetEntryCount());
 }
 
@@ -1414,7 +1468,7 @@
   entry1->Close();
 
   EXPECT_NE(net::OK, OpenNextEntry(&iter, &entry2));
-  MessageLoop::current()->RunAllPending();
+  FlushQueueForTest();
 
   ASSERT_EQ(net::OK, CreateEntry("Something new", &entry2));
   entry2->Close();
@@ -1482,7 +1536,8 @@
   entry1->Close();
   entry2->Close();
   entry3->Close();
-  MessageLoop::current()->RunAllPending();
+  FlushQueueForTest();  // Flushing the Close posts a task to restart the cache.
+  FlushQueueForTest();  // This one actually allows that task to complete.
 
   EXPECT_EQ(0, cache_->GetEntryCount());
 }