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 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;
   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(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;
 }
 
 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);

[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(),
                  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(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);
 }
 
 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) {
-  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