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..4edcf63443447d0c0d24f551529bc64b1da25e85 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,35 @@ void CallCompletionCallback(const net::CompletionCallback& callback,
   callback.Run(error_code);
 }
 
+// See ChainOperationIntoClosure, below.
+void ChainOperationIntoClosureImpl(
+    const base::WeakPtr<disk_cache::SimpleBackendImpl>& backend_weak_ptr,
+    const Callback<int(const net::CompletionCallback&)>& to_chain_operation,
+    const net::CompletionCallback& operation_callback,
+    const Closure& closure) {
+  if (backend_weak_ptr) {

[Philippe, 2013/08/30 13:33:14]

I might be wrong but I believe that we have the guarantee here that the backend
will systematically outlive the callback chain since it owns the callback chain.

You would need the weak pointer though if you were pushing tasks to a message
loop (outliving the backend) rather than the |entries_pending_doom_| queue owned
by the backend.

Wdyt?

[gavinp, 2013/08/30 18:03:10]

I think you're right. Done.

+    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.
+// Note that the |to_chain_operation| has to keep a base::Unretained pointer to
+// the backend, so we add |backend_weak_ptr| and explicitly check it before any
+// operation dereferences the backend.
+Closure ChainOperationIntoClosure(
+    const base::WeakPtr<disk_cache::SimpleBackendImpl>& backend_weak_ptr,
+    const Callback<int(const net::CompletionCallback&)>& to_chain_operation,
+    const net::CompletionCallback& operation_callback,
+    const Closure& closure) {
+  return base::Bind(&ChainOperationIntoClosureImpl, backend_weak_ptr,
+                    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 +296,23 @@ void SimpleBackendImpl::OnDeactivated(const SimpleEntryImpl* entry) {
   active_entries_.erase(entry->entry_hash());
 }
 
+void SimpleBackendImpl::OnDoomStart(const uint64 entry_hash) {

[Philippe, 2013/08/30 13:33:14]

Nit: As much as I like const, this one (and the one line 305) might be slight
overkill :) You can move the one on line 305 to line 307 if you want to keep one
:)

[gavinp, 2013/08/30 18:03:10]

Done.

+  DCHECK_EQ(0u, entries_pending_doom_.count(entry_hash));
+  entries_pending_doom_.insert(
+      base::hash_map<uint64, Closure>::value_type(entry_hash, Closure()));

[Philippe, 2013/08/30 13:33:14]

Nit: any reason not to use the shorter make_pair() here?

[gavinp, 2013/08/30 18:03:10]

No, none. Done.

+}
+
+void SimpleBackendImpl::OnDoomComplete(const 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 +325,20 @@ 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);

[Philippe, 2013/08/30 13:33:14]

Nit: wouldn't it be more common to break after '=' instead as you do on line
333? This comment would also apply to line 341, 359, 369 and 386.

[gavinp, 2013/08/30 18:03:10]

Done.

+  if (it != entries_pending_doom_.end()) {
+    Callback<int(const net::CompletionCallback&)> operation =
+        base::Bind(&SimpleBackendImpl::OpenEntry,
+                   base::Unretained(this), key, entry);
+    it->second = ChainOperationIntoClosure(AsWeakPtr(),
+                                           operation, callback, it->second);
+    return net::ERR_IO_PENDING;
+  }
+  scoped_refptr<SimpleEntryImpl>
+      simple_entry = CreateOrFindActiveEntry(entry_hash, key);
   CompletionCallback backend_callback =
       base::Bind(&SimpleBackendImpl::OnEntryOpenedFromKey,
                  AsWeakPtr(),
@@ -292,14 +352,39 @@ 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(AsWeakPtr(),
+                                           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(AsWeakPtr(),
+                                           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 +507,8 @@ SimpleBackendImpl::DiskStatResult SimpleBackendImpl::InitCacheStructureOnDisk(
 }
 
 scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
+    const uint64 entry_hash,
     const std::string& key) {
-  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 +527,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());
 }