Track entries pending Doom in SimpleCache backend.

net/disk_cache/simple/simple_backend_impl.cc

 // Copyright (c) 2013 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/disk_cache/simple/simple_backend_impl.h"
 
 #include <algorithm>
 #include <cstdlib>
 
 #if defined(OS_POSIX)
@@ skipping 15 matching lines @@
 #include "base/time/time.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/backend_impl.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
 #include "net/disk_cache/simple/simple_entry_impl.h"
 #include "net/disk_cache/simple/simple_index.h"
 #include "net/disk_cache/simple/simple_index_file.h"
 #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;
 using base::SequencedWorkerPool;
 using base::SingleThreadTaskRunner;
 using base::Time;
 using base::DirectoryExists;
 using file_util::CreateDirectory;
 
 namespace {
@@ skipping 143 matching lines @@
     return true;
   }
 }
 
 void CallCompletionCallback(const net::CompletionCallback& callback,
                             int error_code) {
   DCHECK(!callback.is_null());
   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;
   if (result == net::OK)
     UMA_HISTOGRAM_TIMES("SimpleCache.CreationToIndex", creation_to_index);
   else
     UMA_HISTOGRAM_TIMES("SimpleCache.CreationToIndexFail", creation_to_index);
 }
 
 }  // namespace
@@ skipping 50 matching lines @@
 }
 
 int SimpleBackendImpl::GetMaxFileSize() const {
   return index_->max_size() / kMaxFileRatio;
 }
 
 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;
 }
 
 int32 SimpleBackendImpl::GetEntryCount() const {
   // TODO(pasko): Use directory file count when index is not ready.
   return index_->GetEntryCount();
 }
 
 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(),
                  key,
                  entry,
                  simple_entry,
                  callback);
   return simple_entry->OpenEntry(entry, backend_callback);
 }
 
 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);
 }
 
 int SimpleBackendImpl::DoomAllEntries(const CompletionCallback& callback) {
   return DoomEntriesBetween(Time(), Time(), callback);
 }
 
 void SimpleBackendImpl::IndexReadyForDoom(Time initial_time,
                                           Time end_time,
                                           const CompletionCallback& callback,
@@ skipping 102 matching lines @@
         // TODO(pasko): Move PreferedCacheSize() to cache_util.h. Also fix the
         // spelling.
         result.max_size = disk_cache::PreferedCacheSize(available);
     }
     DCHECK(result.max_size);
   }
   return result;
 }
 
 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>()));
   EntryMap::iterator& it = insert_result.first;
   if (insert_result.second)
     DCHECK(!it->second.get());
   if (!it->second.get()) {
     SimpleEntryImpl* entry = new SimpleEntryImpl(
         path_, entry_hash, entry_operations_mode_, this, net_log_);
     entry->SetKey(key);
     it->second = entry->AsWeakPtr();
   }
   DCHECK(it->second.get());
   // It's possible, but unlikely, that we have an entry hash collision with a
   // currently active entry.
   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());
 }
 
 int SimpleBackendImpl::OpenEntryFromHash(uint64 hash,
                                          Entry** entry,
                                          const CompletionCallback& callback) {
   EntryMap::iterator has_active = active_entries_.find(hash);
   if (has_active != active_entries_.end())
     return OpenEntry(has_active->second->key(), entry, callback);
@@ skipping 105 matching lines @@
     const CompletionCallback& callback,
     int error_code) {
   if (error_code == net::ERR_FAILED) {
     OpenNextEntry(iter, entry, callback);
     return;
   }
   CallCompletionCallback(callback, error_code);
 }
 
 }  // namespace disk_cache