Track entries pending Doom in SimpleCache backend.

net/disk_cache/simple/simple_backend_impl.cc

Index: net/disk_cache/simple/simple_backend_impl.cc
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index c8ff759b80c34e2348c26d2e11541ab7c0b91d7d..6ce04461970f85025127e4425aa12adb20c6f562 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -33,6 +33,7 @@
 #include "net/disk_cache/simple/simple_synchronous_entry.h"
 #include "net/disk_cache/simple/simple_util.h"
 
+using base::Callback;
 using base::Closure;
 using base::FilePath;
 using base::MessageLoopProxy;
@@ -196,6 +197,28 @@ void CallCompletionCallback(const net::CompletionCallback& callback,
   callback.Run(error_code);
 }
 
+// See ChainOperationIntoClosure, below.
+void ChainOperationIntoClosureImpl(
+    const Callback<int(const net::CompletionCallback&)>& to_chain_operation,
+    const net::CompletionCallback& operation_callback,
+    const Closure& closure) {
+  const int operation_result = to_chain_operation.Run(operation_callback);
+  if (operation_result != net::ERR_IO_PENDING)
+    operation_callback.Run(operation_result);
+  if (!closure.is_null())
+    closure.Run();
+}
+
+// Returns a Closure that will first run |to_chain_operation|, returning its
+// result to |operation_callback|, and finally run |closure| if it is not null.
+Closure ChainOperationIntoClosure(
+    const Callback<int(const net::CompletionCallback&)>& to_chain_operation,
+    const net::CompletionCallback& operation_callback,
+    const Closure& closure) {
+  return base::Bind(&ChainOperationIntoClosureImpl, to_chain_operation,
+                    operation_callback, closure);
+}
+
 void RecordIndexLoad(base::TimeTicks constructed_since, int result) {
   const base::TimeDelta creation_to_index = base::TimeTicks::Now() -
                                             constructed_since;
@@ -266,6 +289,22 @@ void SimpleBackendImpl::OnDeactivated(const SimpleEntryImpl* entry) {
   active_entries_.erase(entry->entry_hash());
 }
 
+void SimpleBackendImpl::OnDoomStart(uint64 entry_hash) {
+  DCHECK_EQ(0u, entries_pending_doom_.count(entry_hash));
+  entries_pending_doom_.insert(std::make_pair(entry_hash, Closure()));
+}
+
+void SimpleBackendImpl::OnDoomComplete(uint64 entry_hash) {
+  DCHECK_EQ(1u, entries_pending_doom_.count(entry_hash));
+  base::hash_map<uint64, base::Closure>::iterator it =
+      entries_pending_doom_.find(entry_hash);
+  Closure to_run_closure = it->second;
+  entries_pending_doom_.erase(it);
+
+  if (!to_run_closure.is_null())
+    to_run_closure.Run();
+}
+
 net::CacheType SimpleBackendImpl::GetCacheType() const {
   return net::DISK_CACHE;
 }
@@ -278,7 +317,19 @@ int32 SimpleBackendImpl::GetEntryCount() const {
 int SimpleBackendImpl::OpenEntry(const std::string& key,
                                  Entry** entry,
                                  const CompletionCallback& callback) {
-  scoped_refptr<SimpleEntryImpl> simple_entry = CreateOrFindActiveEntry(key);
+  const uint64 entry_hash = simple_util::GetEntryHashKey(key);
+
+  base::hash_map<uint64, base::Closure>::iterator it =
+      entries_pending_doom_.find(entry_hash);
+  if (it != entries_pending_doom_.end()) {
+    Callback<int(const net::CompletionCallback&)> operation =
+        base::Bind(&SimpleBackendImpl::OpenEntry,
+                   base::Unretained(this), key, entry);
+    it->second = ChainOperationIntoClosure(operation, callback, it->second);

[Deprecated (see juliatuttle), 2013/09/03 16:15:11]

This is really clever, but I think it's less clear and uses more overhead (an
extra closure for every callback past the first) than making
entries_pending_doom_ a map<uint64, vector<Closure> >.

[Philippe, 2013/09/03 16:24:33]

I agree at a glance that a map/multimap would look clearer. I'm not sure how
this would look like in practice though given the fact that the callbacks can be
executed asynchronously (i.e. it seems that we can't just iterate over the
callbacks vector and run each callback as we would do synchronously). What do
you think?

[Philippe, 2013/09/03 16:25:58]

s/how this/what this :)

[Randy Smith (Not in Mondays), 2013/09/03 19:04:19]

I think I'd prefer a map to a vector as Thomas suggests over the current code,
though I reserve the right to change my mind if the new code looks horrid :-}. 
But that's because I don't see what's stopping us from iterating over the
callbacks and calling each one.  The ones for which that will produce an async
post of some sort will have mechanisms in place (pending_operations_) to
serialize what needs to be serialized.  Can you give me a clearer idea of what
problems you see coming from the naive for () { callback.Run(); } loop?

[Philippe, 2013/09/04 08:55:49]

Right, this should work actually thanks to the operations serialization
mechanism in SimpleEntryImpl.

[gavinp, 2013/09/11 15:25:58]

The latest upload uses a vector, and the code is very readable. Take a look.

+    return net::ERR_IO_PENDING;
+  }
+  scoped_refptr<SimpleEntryImpl> simple_entry =
+      CreateOrFindActiveEntry(entry_hash, key);
   CompletionCallback backend_callback =
       base::Bind(&SimpleBackendImpl::OnEntryOpenedFromKey,
                  AsWeakPtr(),
@@ -292,14 +343,37 @@ int SimpleBackendImpl::OpenEntry(const std::string& key,
 int SimpleBackendImpl::CreateEntry(const std::string& key,
                                    Entry** entry,
                                    const CompletionCallback& callback) {
-  DCHECK(key.size() > 0);
-  scoped_refptr<SimpleEntryImpl> simple_entry = CreateOrFindActiveEntry(key);
+  DCHECK_GT(key.size(), 0u);
+  const uint64 entry_hash = simple_util::GetEntryHashKey(key);
+
+  base::hash_map<uint64, base::Closure>::iterator it =
+      entries_pending_doom_.find(entry_hash);
+  if (it != entries_pending_doom_.end()) {
+    Callback<int(const net::CompletionCallback&)> operation =
+        base::Bind(&SimpleBackendImpl::CreateEntry,
+                   base::Unretained(this), key, entry);
+    it->second = ChainOperationIntoClosure(operation, callback, it->second);
+    return net::ERR_IO_PENDING;
+  }
+  scoped_refptr<SimpleEntryImpl> simple_entry =
+      CreateOrFindActiveEntry(entry_hash, key);
   return simple_entry->CreateEntry(entry, callback);
 }
 
 int SimpleBackendImpl::DoomEntry(const std::string& key,
                                  const net::CompletionCallback& callback) {
-  scoped_refptr<SimpleEntryImpl> simple_entry = CreateOrFindActiveEntry(key);
+  const uint64 entry_hash = simple_util::GetEntryHashKey(key);
+
+  base::hash_map<uint64, base::Closure>::iterator it =
+      entries_pending_doom_.find(entry_hash);
+  if (it != entries_pending_doom_.end()) {
+    Callback<int(const net::CompletionCallback&)> operation =
+        base::Bind(&SimpleBackendImpl::DoomEntry, base::Unretained(this), key);
+    it->second = ChainOperationIntoClosure(operation, callback, it->second);
+    return net::ERR_IO_PENDING;
+  }
+  scoped_refptr<SimpleEntryImpl> simple_entry =
+      CreateOrFindActiveEntry(entry_hash, key);
   return simple_entry->DoomEntry(callback);
 }
 
@@ -422,9 +496,8 @@ SimpleBackendImpl::DiskStatResult SimpleBackendImpl::InitCacheStructureOnDisk(
 }
 
 scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
+    const uint64 entry_hash,
     const std::string& key) {

[Randy Smith (Not in Mondays), 2013/09/03 19:04:19]

Suggestion: DCHECK(entry_hash, simple_util::GetEntryHashKey(key))?

[gavinp, 2013/09/11 15:25:58]

Done.

-  const uint64 entry_hash = simple_util::GetEntryHashKey(key);
-
   std::pair<EntryMap::iterator, bool> insert_result =
       active_entries_.insert(std::make_pair(entry_hash,
                                             base::WeakPtr<SimpleEntryImpl>()));
@@ -443,7 +516,7 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
   if (key != it->second->key()) {
     it->second->Doom();
     DCHECK_EQ(0U, active_entries_.count(entry_hash));
-    return CreateOrFindActiveEntry(key);
+    return CreateOrFindActiveEntry(entry_hash, key);
   }
   return make_scoped_refptr(it->second.get());
 }